Closed Bug 1561548 Opened 1 year ago Closed 1 year ago

Remove -Wmaybe-uninitialized warning in pkix_pl_ldapdefaultclient.c

Categories

(NSS :: Build, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: giulio.benetti, Assigned: giulio.benetti)

Details

Attachments

(1 file, 1 obsolete file)

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.

This patch fixes the warning.

Assignee: nobody → giulio.benetti
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Attachment #9074150 - Flags: review?(jjones)
Comment on attachment 9074150 [details] [diff] [review]
0001-Bug-1561548-Remove-Wmaybe-uninitialized-warning-in.patch

Moving to Marcus (hope you don't mind!)
Attachment #9074150 - Flags: review?(jjones) → review?(marcus.apb)
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?
Attachment #9074150 - Flags: review?(marcus.apb) → review-
Flags: needinfo?(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

Flags: needinfo?(giulio.benetti)

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,

Flags: needinfo?(giulio.benetti)

Here is the requested patch.

Attachment #9074150 - Attachment is obsolete: true
Flags: needinfo?(giulio.benetti)
Comment on attachment 9079264 [details] [diff] [review]
v2-0001-Bug-1561548-Remove-Wmaybe-uninitialized-warning-i.patch

Thanks Giulio.
Attachment #9079264 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.46
You need to log in before you can comment on or make changes to this bug.