Re: [edk2-devel] [PATCH 1/1] CryptoPkg/TlsLib: fix tls cipher configuration

["Gerd Hoffmann" <kraxel@redhat.com>]

  Hi,

> > +  CHAR8             *CipherString;
> > +  CHAR8             *CipherStringPosition;
> > +
> > +  STACK_OF (SSL_CIPHER)      *OpensslCipherStack;
> 
> Surprisingly, this does pass uncrustify. :) OK.

Wasn't my idea, uncrustify actually formatted it that way for me ;)

> > -    Mapping = TlsGetCipherMapping (CipherId[Index]);
> > -    if (Mapping == NULL) {
> > +    for (StackIdx = 0; StackIdx < sk_SSL_CIPHER_num (OpensslCipherStack); StackIdx++) {
> 
> Surprisingly, StackIdx really does have to be INT32 type here; through
> multiple macro definitions, sk_SSL_CIPHER_num() seems to expand to
> OPENSSL_sk_num(), and that returns a plain "int". OK.

Yep.  Initially I had UINTN, but that caused CI errors due to the type
mismatch ...

> Then this loop will never run (which is fine), but *after* the loop, our
> condition for "not found" will not hold, because StackIdx will be zero,
> but sk_SSL_CIPHER_num() will return (-1).

Good point, missed that.

> but it's probably better to just skip the whole outer loop if
> "OpensslCipherStack" is NULL (as no lookup will succeed).

Agree, will do it that way.

> (4) Why AsciiStrnLenS(), and not AsciiStrLen()?

No good reason.  I just looked up the edk2 name of strlen using
'git grep strlen CryptoPkg/Library/Include', and this is the
result I got ;)

> > -    Mapping = MappedCipher[Index];
> > +    OpensslCipher         = MappedCipher[Index];
> > +    OpensslCipherName     = SSL_CIPHER_get_name (OpensslCipher);
> > +    OpensslCipherNameSize = AsciiStrnLenS (OpensslCipherName, MAX_STRING_SIZE);
> 
> (5) Better call this "OpensslCipherNameLength" ("Size" suggests
> including the terminating NUL).

OK.

> (8) Can you
> 
> - assign <https://bugzilla.tianocore.org/show_bug.cgi?id=2541> to yourself,
> 
> - link this patch into a comment on that BZ,
> 
> - and reference the BZ URL in the commit message?

Will do.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109311): https://edk2.groups.io/g/devel/message/109311
Mute This Topic: https://groups.io/mt/101657203/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-