1197314 - remove PR_snprintf calls in security/manager/ssl/

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Giovanny Gongora [:gioyik]]

Attachment: 1197314.patch

Comment 2

[Giovanny Gongora [:gioyik]]

Hi Nathan,

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

Thanks

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Aniket Vyas]

Attachment: bug1197314.patch

Patch to address all the comments and requirements. Please let me know if any changes are needed.

Comment 5

[Nathan Froyd [:froydnj]]

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.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 7

[Aniket Vyas]

Attachment: 1197314.patch

Hi Nathan, David. Thanks for your patience for the review, have attached an updated patch with necessary changes. Please have a look.

Comment 8

[Nathan Froyd [:froydnj]]

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.

Comment 9

[Aniket Vyas]

Attachment: 1197314.patch

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

Comment 10

[Aniket Vyas]

Also, how do I reassign this bug (or any other that I work on) to myself ? 

Thanks,
Aniket

Comment 11

[Nathan Froyd [:froydnj]]

(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.

Comment 12

[Nathan Froyd [:froydnj]]

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.

Comment 13

[Aniket Vyas]

Attachment: 1197314.patch

Hey Nathan, 

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

Regards,
Aniket

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8674517 [details] [diff] [review]
1197314.patch

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

Thank you!

Comment 15

[Aniket Vyas]

Hey Nathan, 

    Do you want me to send the patch for testing ? 

Thanks,
Aniket

Comment 16

[Nathan Froyd [:froydnj]]

If you have try access, please go ahead!

Comment 17

[aliyua7]

Hello,I would like to work on this bug

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(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?

Comment 19

[Aniket Vyas]

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.

Comment 20

[Aniket Vyas]

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

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 22

[Aniket Vyas]

Thanks David ! Appreciate the help.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc43503ff50b