1561548 - Remove -Wmaybe-uninitialized warning in pkix_pl_ldapdefaultclient.c

Status: RESOLVED FIXED

(NSS :: Build, defect, P2)

Description

[Giulio Benetti]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36

Steps to reproduce:

Build with:
make nss_build_all BUILD_OPT=1 ALLOW_OPT_CODE_SIZE=1 OPT_CODE_SIZE=1

Actual results:

pkix_pl_ldapdefaultclient.c:376:21: error: ‘*((void )&msg+32).resultCode.data’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
if ((ldapBindResponse->resultCode.data) == SUCCESS) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Expected results:

No warning.

Comment 1

[Giulio Benetti]

Attachment: 0001-Bug-1561548-Remove-Wmaybe-uninitialized-warning-in.patch

This patch fixes the warning.

Comment 2

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9074150 [details] [diff] [review]
0001-Bug-1561548-Remove-Wmaybe-uninitialized-warning-in.patch

Moving to Marcus (hope you don't mind!)

Comment 3

[Marcus Burghardt]

Comment on attachment 9074150 [details] [diff] [review]
0001-Bug-1561548-Remove-Wmaybe-uninitialized-warning-in.patch

Review of attachment 9074150 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Giulio.
Please, take a look in the bellow comment.

::: lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c
@@ +362,4 @@
>          decode.data = (unsigned char *)(client->rcvBuf);
>          decode.len = bufLen;
>  
> +        ldapBindResponse->resultCode.data = NULL;

So, the if bellow (line 372) will never find SUCCESS, as you are always overriding the value received from msg.protocolOp.op.bindResponseMsg.
Just initialize "resultCode.data" may be enough to solve this warning.
Example:

LDAPBindResponse *ldapBindResponse = NULL;
ldapBindResponse->resultCode.data = NULL;

And the rest remains untouched.
Could you do that, please?

Comment 4

[Giulio Benetti]

Hi Marcus,

IMHO I'm not always overriding value received from msg.protocolOp.op.bindResponseMsg, I'm only pointing to it.
Then, before calling:
PKIX_CHECK(pkix_pl_LdapDefaultClient_DecodeBindResponse
(client->arena, &decode, &msg, &rv, plContext),
PKIX_LDAPDEFAULTCLIENTDECODEBINDRESPONSEFAILED);

I set:
ldapBindResponse->resultCode.data = NULL;

so if pkix_pl_LdapDefaultClient_DecodeBindResponse() changes or not "ldapBindResponse->resultCode.data" it should be ok.

What you're suggesting is not possible due to null-pointer dereferencing when:
LDAPBindResponse *ldapBindResponse = NULL;
ldapBindResponse->resultCode.data = NULL;

But I can move:
ldapBindResponse->resultCode.data = NULL;
right after:
LDAPBindResponse *ldapBindResponse = &msg.protocolOp.op.bindResponseMsg;
and this ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ is only a way to short this:
ldapBindResponse->resultCode.data = NULL;

Best regards

Comment 5

[Marcus Burghardt]

Hi Giulio,

Yes, this is the point. : )
Move ldapBindResponse->resultCode.data = NULL right after LDAPBindResponse *ldapBindResponse = &msg.protocolOp.op.bindResponseMsg;
This is enough.

Could you update the patch, please?

Thanks,

Comment 6

[Giulio Benetti]

Attachment: v2-0001-Bug-1561548-Remove-Wmaybe-uninitialized-warning-i.patch

Here is the requested patch.

Comment 7

[Marcus Burghardt]

Comment on attachment 9079264 [details] [diff] [review]
v2-0001-Bug-1561548-Remove-Wmaybe-uninitialized-warning-i.patch

Thanks Giulio.

Comment 8

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/64ddab1e17c8616b15a5f275b7019453863e4907