Closed Bug 1400591 Opened 7 years ago Closed 7 years ago

ssl3con.c signed/unsigned compilation warnings

Categories

(NSS :: Libraries, defect, P1)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jeanluc.bonnafoux, Assigned: jeanluc.bonnafoux)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1400591 - ssl3con.c signed/unsigned compilation warnings, r?fkiefer
44 bytes, text/x-phabricator-request
Details | Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053622

Steps to reproduce:

Compiling Firefox trunk on WIN10 and Visual Studio 2017 community edition, there are several "incompatible signed/unsigned" warnings generated by the compiler.

security/nss/lib/ssl/ssl3con.c:4361 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:5247 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:5414 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:6784 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:7118 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:8062 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:8768 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:9682 [C4018] '<' : incompatibilité signed/unsigned
security/nss/lib/ssl/ssl3con.c:10896 [C4018] '<' : incompatibilité signed/unsigned


Actual results:

Code compiled OK.


Expected results:

Code should have compiled without warnings.
Priority: -- → P3
Hello,
Could you please assign this bug to me?
Thanks,
Attachment #8909451 - Flags: review?(franziskuskiefer)
Assignee: nobody → jbannon
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: P3 → P1
Comment on attachment 8909451 [details] [diff] [review]
ssl3con.c signed/unsigned compilation warnings, review:fkiefer

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

This is a little hard to review without more context. Would you mind uploading the next revision to https://phabricator.services.mozilla.com/? (just link it to your bugzilla account)

::: security/nss/lib/ssl/ssl3con.c
@@ +6608,5 @@
>  {
>      PRUint32 temp;
>      PRBool suite_found = PR_FALSE;
>      int i;
> +    unsigned int ui;

What about the other for loop?
`for (i = 0; i < ssl_V3_SUITES_IMPLEMENTED; i++)`

@@ +7097,5 @@
>  
>      SECItem dh_p = { siBuffer, NULL, 0 };
>      SECItem dh_g = { siBuffer, NULL, 0 };
>      SECItem dh_Ys = { siBuffer, NULL, 0 };
> +	PRInt32 dh_p_bits;

Indentation

@@ +8052,5 @@
>  ssl3_NegotiateCipherSuite(sslSocket *ss, const SECItem *suites,
>                            PRBool initHashes)
>  {
>      int j;
> +	unsigned int i;

Indentation

@@ +8677,5 @@
>      int errCode = SSL_ERROR_RX_MALFORMED_CLIENT_HELLO;
>      SSL3AlertDescription desc = illegal_parameter;
>      SECStatus rv;
>      unsigned int i;
> +	unsigned int uj;

Indentation
Attachment #8909451 - Flags: review?(franziskuskiefer)
Hello,

I have create a mozilla phabricator account and linked it to my bugzilla account.
What is the best way to re-submit a patch on this ticket?

for (i = 0; i < ssl_V3_SUITES_IMPLEMENTED; i++) : it does not generate a warning because ssl_V3_SUITES_IMPLEMENTED is the result of a #define and i is of int type.

Thanks,
I have fixed indentation following previous review.
Attachment #8909451 - Attachment is obsolete: true
Attachment #8911467 - Flags: review?(franziskuskiefer)
Hello,

Would you like me to submit patch proposal through phabricator / arcanist?

Thanks,
If you put it up on phabricator would be easier to review (because it has more context).
Hello,

Arcanist does not seem to accept fkiefer or franziscuskiefer as reviewer of my patch proposal.
Which value should i use please for the Reviewer field ?
Thanks,
Hello,

I have submitted patch into phabricator (reviewer: ttaubert),
https://phabricator.services.mozilla.com/D93

Thanks,
Hello,
is the attached Phabricator patch usable for validation?
Thanks,
(In reply to jbonnafo from comment #10)
> is the attached Phabricator patch usable for validation?

Yes, thanks! I reviewed the patch and left a few comments.
Hello,
Thanks for the review.
I have re-based my code and tried a new patch proposal.
It is available in Phabricator: https://phabricator.services.mozilla.com/D124

I have forgotten to attach bug ID into the patch so that it is not attached to this bug.
Is it possible to link in Phabricator the patch proposal to this bug ?

Thanks,
(In reply to jbonnafo from comment #12)
> I have re-based my code and tried a new patch proposal.
> It is available in Phabricator: https://phabricator.services.mozilla.com/D124

Can you please update the existing revision D93? And close the new D124? Otherwise it makes it a lot harder to review changes between revisions and look at what comments I gave last time.

You can update an existing revision with `arc diff --update D93`.

> Is it possible to link in Phabricator the patch proposal to this bug ?

It's automatically linked, seems to work?
Flags: needinfo?(jbannon)
Hello,

I have updated revision D93 in phabricator following your advise.

I am also going to close unnecessary D124.

Could it be possible to obsolete the first non phabricator patch proposal?

Thanks,
Attachment #8918205 - Attachment is obsolete: true
Attachment #8911467 - Attachment is obsolete: true
Attachment #8911467 - Flags: review?(franziskuskiefer)
Hello,

What can i do in order to move forward on this issue ?

Thanks,
Hello,

Could you please tell me what the next step would be in order to solve this issue?

Thanks,
(In reply to jbonnafo from comment #16)
> Could you please tell me what the next step would be in order to solve this
> issue?

My bad, I let it slip, sorry :( I left a bunch of comments, and will review the next revision immediately.
Flags: needinfo?(jbannon)
Hello,

Thanks for the code review.
I have amended the patch proposal accordingly.

Thanks,
Reviewed the latest revision. One last issue that needs fixing, then it should be good to go.
Hello,

I have amended the patch proposal. Seems arc has also taken another change i did today, sorry.

Thanks,
https://hg.mozilla.org/projects/nss/rev/f56ef220b862
Assignee: jbannon → jeanluc.bonnafoux
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.35
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: