remove PR_snprintf calls in security/manager/ssl/

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: froydnj, Assigned: vyas45, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

Steps:

1. Find all calls to PR_snprintf in security/manager/ssl/.
2. Add #include "mozilla/Snprintf.h" to the file.
3. Replace PR_snprintf(X, sizeof(X), ...) calls with snprintf_literal(X, ...).
4. Replace other PR_snprintf calls with snprintf.
5. Remove any #include "prprf.h" lines.
Posted patch 1197314.patch (obsolete) — Splinter Review
Attachment #8651451 - Flags: review?(nfroyd)
Hi Nathan,

I attached a patch for this bug.Let me know the changes this one needs.

Thanks
Assignee: nobody → gioyik
Comment on attachment 8651451 [details] [diff] [review]
1197314.patch

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

Thanks for the patch!  A few mistranslations to snprintf_literal are noted below.

::: security/manager/ssl/nsNSSCertHelper.cpp
@@ +213,1 @@
>  			      one, separator, two);

Does this actually compile?  It would certainly be nice if it did, but somehow I doubt that it does.  Please use snprintf here and fixup the indentation as well.

@@ +217,1 @@
>  			      separator, val);

Likewise here.

@@ +221,5 @@
>        nsAutoString unknownText;
>        nssComponent->GetPIPNSSBundleString("CertUnknown", 
>                                            unknownText);
>        if (first) {
> +        written = snprintf_literal(&buf[len], "%s",

Likewise here.

@@ +230,1 @@
>                                separator, 

Likewise here.

@@ +640,5 @@
>  
>    uint32_t i;
>    char buffer[5];
>    for (i=0; i<data->len; i++) {
> +    snprintf_literal(buffer, "%02x ", data->data[i]);

Nice catch on not making this snprintf.

@@ +992,1 @@
>  		      "{%.2x%.2x%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x%.2x%.2x%.2x%.2x}",

Please fixup the indentation for the arguments to this call.

::: security/manager/ssl/nsNTLMAuthModule.cpp
@@ +188,5 @@
>      strcpy(line, "    ");
>      for (i=0; i<count; ++i)
>      {
>        int len = strlen(line);
> +      snprintf_literal(line + len, "0x%02x ", int(buf[i]));

All of the snprintf_literal calls in this function need to be calls to snprintf.
Attachment #8651451 - Flags: review?(nfroyd) → feedback+
Posted patch bug1197314.patch (obsolete) — Splinter Review
Patch to address all the comments and requirements. Please let me know if any changes are needed.
Attachment #8672874 - Flags: review?(nfroyd)
Comment on attachment 8672874 [details] [diff] [review]
bug1197314.patch

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

This patch looks good overall; just some minor formatting issues.

::: security/manager/ssl/nsNSSCertHelper.cpp
@@ +209,5 @@
>          unsigned long one = std::min(val/40, 2UL); // never > 2
>          unsigned long two = val - (one * 40);
>  
> +        written = snprintf(&buf[len], sizeof(buf)-len, "%lu%c%lu", 
> +			               one, separator, two);

This line is indented too far to the right; the |one| needs to start directly under |&buf[len]|.  This might be because the line is indented with tabs and your editor is set up to display tabs as four spaces rather than 8?  You should configure your editor to use tabs when you indent (but not convert tabs to spaces when you save!); you can see our typical emacs/vim modelines here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

If you use one of those editors, please feel free to add the appropriate modeline to files that you edit if they don't already have one.  Some developers set tabs to display as 20 or 40 spaces, so it's painfully obvious when tabs get used accidentally, but that may make viewing files that don't follow the formatting rules...problematic. ;)

@@ +988,5 @@
>  	  char buf[40];
>  	  unsigned char *d = guid.data;
> +	  snprintf_literal(buf, "{%.2x%.2x%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x%\
> +                       .2x%.2x%.2x%.2x}", d[3], d[2], d[1], d[0], d[5], d[4], d[7], 
> +                       d[6], d[8], d[9], d[10], d[11], d[12], d[13], d[14], d[15]);

Minor nit here; I think we'd normally have the rest of the ".2x%.2x..." string start directly underneath |buf|, and the line starting with |d[6]| likewise.
Attachment #8672874 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8672874 [details] [diff] [review]
bug1197314.patch

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

::: security/manager/ssl/nsNSSCertHelper.cpp
@@ +214,3 @@
>        }
>        else {
> +        written = snprintf(&buf[len], sizeof(buf)-len, "%c%lu", 

Drive-by nit: also feel free to remove trailing spaces on lines this patch modifies
Posted patch 1197314.patch (obsolete) — Splinter Review
Hi Nathan, David. Thanks for your patience for the review, have attached an updated patch with necessary changes. Please have a look.
Attachment #8672874 - Attachment is obsolete: true
Attachment #8673352 - Flags: review?(nfroyd)
Comment on attachment 8673352 [details] [diff] [review]
1197314.patch

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

Thanks for revising the patch; unfortunately there needs to be another round.  Thanks for your patience through all of this!

::: security/manager/ssl/DataStorage.cpp
@@ -16,5 @@
>  #include "nsITimer.h"
>  #include "nsNetUtil.h"
>  #include "nsStreamUtils.h"
>  #include "nsThreadUtils.h"
> -#include "prprf.h"

Please don't remove this; I realize that PR_snprintf etc. aren't used in this file, but this header likely brings in other things like PR_USEC_PER_SEC and the like.  I should have been more explicit in my instructions in comment 0 to only remove such #includes in the files that were changed, so that's my mistake in describing what needs to be done here.

Having the correct header(s) included is a worthy goal, but it's not appropriate in the fix for this bug.

::: security/manager/ssl/nsNSSCertificate.cpp
@@ -6,5 @@
>  #include "nsNSSCertificate.h"
>  
>  #include "prmem.h"
>  #include "prerror.h"
> -#include "prprf.h"

Same comment as above applies here.

::: security/manager/ssl/nsPKCS12Blob.cpp
@@ -7,5 @@
>  
>  #include "pkix/pkixtypes.h"
>  
>  #include "prmem.h"
> -#include "prprf.h"

Same comment applies here.

::: security/manager/ssl/nsSiteSecurityService.cpp
@@ -22,5 @@
>  #include "pkix/pkixtypes.h"
>  #include "plstr.h"
>  #include "mozilla/Logging.h"
>  #include "prnetdb.h"
> -#include "prprf.h"

Same comment applies here.

::: security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
@@ -10,5 @@
>  #include "nss.h"
>  #include "pkix/pkixtypes.h"
>  #include "pkixtestutil.h"
>  #include "prerr.h"
> -#include "prprf.h"

Same comment applies here.
Attachment #8673352 - Flags: review?(nfroyd) → feedback+
Attachment #8651451 - Attachment is obsolete: true
Posted patch 1197314.patch (obsolete) — Splinter Review
Hi Nathan,

       Thanks for your comments. Have updated the patch to revert the "header removals".

       There is one exception though. In the comments that you provided the last file, security/manager/ssl/tests/gtest/OCSPCacheTest.cpp does have a few changes from PR_snprintf to snprintf : 

-  PR_snprintf(reinterpret_cast<char*>(serialBuf), sizeof(serialBuf), "%04d", i);
+  snprintf(reinterpret_cast<char*>(serialBuf), sizeof(serialBuf), "%04d", i);


and a couple more.. Is it not worthy of the header removal ? The current patch doesn't remove but please let me know if it calls for. 

Thanks,
Aniket
Attachment #8673352 - Attachment is obsolete: true
Attachment #8673975 - Flags: review?(nfroyd)
Also, how do I reassign this bug (or any other that I work on) to myself ? 

Thanks,
Aniket
Flags: needinfo?(nfroyd)
(In reply to Aniket Vyas from comment #9)
>        There is one exception though. In the comments that you provided the
> last file, security/manager/ssl/tests/gtest/OCSPCacheTest.cpp does have a
> few changes from PR_snprintf to snprintf : 
> 
> -  PR_snprintf(reinterpret_cast<char*>(serialBuf), sizeof(serialBuf),
> "%04d", i);
> +  snprintf(reinterpret_cast<char*>(serialBuf), sizeof(serialBuf), "%04d",
> i);

You're right!  Sorry for the thinko on my part.
Flags: needinfo?(nfroyd)
Comment on attachment 8673975 [details] [diff] [review]
1197314.patch

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

Thanks for the changes!  I think the below changes are trivial enough that you can just upload a new patch with those changes.

(You might not be seeing compile errors due to the omitted #includes because of the way we build things; it's considered good practice to always #include what you need in .cpp files, though.)

::: security/manager/ssl/nsNTLMAuthModule.cpp
@@ +187,5 @@
>      strcpy(line, "    ");
>      for (i=0; i<count; ++i)
>      {
>        int len = strlen(line);
> +      snprintf(line + len, sizeof(line) - len, "0x%02x ", int(buf[i]));

I think the changes in this file require an #include "mozilla/Snprintf.h" somewhere.

::: security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
@@ +84,5 @@
>  {
>    SCOPED_TRACE("");
>    for (int i = 0; i < MaxCacheEntries; i++) {
>      uint8_t serialBuf[8];
> +    snprintf(reinterpret_cast<char*>(serialBuf), sizeof(serialBuf), "%04d", i);

Likewise, I don't see an #include "mozilla/Snprintf.h" here.

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
@@ +68,5 @@
>  SECStatus
>  ReadFileToBuffer(const char* basePath, const char* filename, char (&buf)[N])
>  {
>    static_assert(N > 0, "input buffer too small for ReadFileToBuffer");
> +  if (snprintf(buf, N - 1, "%s/%s", basePath, filename) == 0) {

Same thing here.
Attachment #8673975 - Flags: review?(nfroyd) → review+
Assignee: gioyik → vyas45
Posted patch 1197314.patchSplinter Review
Hey Nathan, 

      Thanks for the comments. Have updated the patch and the commit message. 

Regards,
Aniket
Attachment #8673975 - Attachment is obsolete: true
Attachment #8674517 - Flags: review?(nfroyd)
Comment on attachment 8674517 [details] [diff] [review]
1197314.patch

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

Thank you!
Attachment #8674517 - Flags: review?(nfroyd) → review+
Hey Nathan, 

    Do you want me to send the patch for testing ? 

Thanks,
Aniket
Flags: needinfo?(nfroyd)
If you have try access, please go ahead!
Flags: needinfo?(nfroyd)
Hello,I would like to work on this bug
(In reply to aliyua7 from comment #17)
> Hello,I would like to work on this bug

This is probably not a good bug for you to work on as it's already assigned and has a reviewed patch. It just needs to be pushed over the finish line, really.

Aniket, did you get try access to test out this bug? Is there something I can do to help you move forward with this?
Flags: needinfo?(vyas45)
Hi David, 

    Sorry about the delay , I was out on a break. Will get the patch going on try servers leading to the commit. Would be my first time so would ping back if I get stuck somewhere.
Flags: needinfo?(vyas45)
Hi David, 

 
     I am sorry have not been able to get to the commit part and would need some help pushing it in. 

Thanks,
Aniket
Flags: needinfo?(dkeeler)
Thanks for letting me know.
I pushed a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d77a539bcfd (with some minor style nits addressed)
That looked good, so inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc43503ff50b
Flags: needinfo?(dkeeler)
Thanks David ! Appreciate the help.
https://hg.mozilla.org/mozilla-central/rev/fc43503ff50b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.