1197313 - remove PR_snprintf calls in netwerk/

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nathan Froyd [:froydnj]]

Steps:

1. Find all calls to PR_snprintf in netwerk/.
2. Add #include "mozilla/Snprintf.h" to the file(s).
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: 1197313.patch

Comment 2

[Giovanny Gongora [:gioyik]]

Hi Nathan,

I attached a patch for this. Could you review it? I am sure it needs changes with format string characters, so let me know how to fix them.

Assigning this issue to myself to not loose/forget it.

Thanks

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8651449 [details] [diff] [review]
1197313.patch

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

Thanks for the patch!  A few minor format string fixes below, and a few mistranslated functions.

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +175,5 @@
>    file->AppendNative(nsPrintfCString("%X", dir1));
>    file->AppendNative(nsPrintfCString("%X", dir2));
>  
>    char leaf[64];
> +  snprintf_literal(leaf, "%014llX-%X", hash, generation);

The format string here should be "%014" PRIx64 "-%X".

@@ +410,5 @@
>      file->AppendNative(NS_LITERAL_CSTRING("placeholder"));
>  
>      for (generation = 0; ; ++generation)
>      {
> +      snprintf_literal(leaf, "%014llX-%X", hash, generation);

Likewise here.

@@ +424,5 @@
>      }
>    }
>    else
>    {
> +    snprintf_literal(leaf, "%014llX-%X", hash, generation);

Likewise here.

::: netwerk/protocol/http/Http2Session.cpp
@@ +218,5 @@
>          *line = 0;
>          LOG5(("%s", linebuf));
>        }
>        line = linebuf;
> +      snprintf_literal(line, "%08X: ", index);

The two snprintf_literal calls in this function should be snprintf calls.

@@ +226,1 @@
>                  (reinterpret_cast<const uint8_t *>(data))[index]);

Please fix up the indentation of this line.

::: netwerk/protocol/http/SpdySession31.cpp
@@ +176,5 @@
>          *line = 0;
>          LOG5(("%s", linebuf));
>        }
>        line = linebuf;
> +      snprintf(line, "%08X: ", index);

It looks like you have forgotten to specify a maximum # of characters to print here.

@@ +184,1 @@
>                  ((unsigned char *)data)[index]);

This should be an snprintf call, since |line| is not a character array.  Please fix that, and please remember to fix the indentation too.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +841,5 @@
>      }
>  
>      if (mResuming) {
>          char byteRange[32];
> +        snprintf_literal(byteRange, "bytes=%llu-", mStartPos);

The format string here should be "bytes=%" PRIu64 "-".

@@ +2427,5 @@
>          return NS_ERROR_FAILURE;
>      }
>  
>      char buf[64];
> +    snprintf_literal(buf, "bytes=%lld-", partialLen);

Likewise here, except it should be "bytes=%" PRId64 "-".

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1710,5 @@
>                      // Values below 10 require zero padding.
>                      qval_str = "%s%s;q=0.%02u";
>                  }
>  
> +                wrote = snprintf_literal(p2, available, qval_str, comma, token, u);

The two snprintf_literal calls here should be calls to snprintf instead.

::: netwerk/streamconv/converters/ParseFTPList.cpp
@@ +490,1 @@
>                            "%lld", fsz);

The format string here should be "%" PRId64.

Comment 4

[Madhurima]

I would like to work on this bug, could I please get further information on this?

Comment 5

[Madhurima]

Attachment: Made all the required changes according to the comment 3

Please review the patch and let me know whether anymore changes are required.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8671396 [details] [diff] [review]
Made all the required changes according to the comment 3

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

This looks good, just a few minor changes.  Please fix those up, upload a new patch, and ask me for review again.  Assuming everything looks good with that version, I'll take care of landing in for you.

::: netwerk/protocol/http/Http2Session.cpp
@@ +218,5 @@
>          *line = 0;
>          LOG5(("%s", linebuf));
>        }
>        line = linebuf;
> +      snprintf(line, "%08X: ", index);

It looks like you deleted the maximum length for snprintf here.

@@ +223,3 @@
>        line += 10;
>      }
> +    snprintf(line, "%02X ", (reinterpret_cast<const uint8_t *>(data))[index]);

...and here.

::: netwerk/protocol/http/SpdySession31.cpp
@@ +181,3 @@
>        line += 10;
>      }
> +    snprintf(line, "%02X ", ((unsigned char *)data)[index]);

...and here as well.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2427,5 @@
>          return NS_ERROR_FAILURE;
>      }
>  
>      char buf[64];
> +    snprintf_literal(buf, "bytes=%" PRId+64 "-", partialLen);

This should be PRId64 (no '+' character).

::: netwerk/protocol/http/nsHttpDigestAuth.cpp
@@ +261,5 @@
>      nsCOMPtr<nsISupportsPRUint32> v(do_QueryInterface(*sessionState));
>      if (v) {
>        uint32_t nc;
>        v->GetData(&nc);
> +      snprintf(nonce_count, "%08x", ++nc);

This should be snprintf_literal.

::: netwerk/streamconv/converters/ParseFTPList.cpp
@@ -484,5 @@
>                 * So its rounded up to the next block, so what, its better
>                 * than not showing the size at all.
>                */
>                uint64_t fsz = uint64_t(strtoul(tokens[1], (char **)0, 10) * 512);
> -              PR_snprintf(result->fe_size, sizeof(result->fe_size), 

Did you do you diff from a clean copy of mozilla-central?  This line is incomplete, so I'm not sure what code you based your patch off of.

Comment 7

[Madhurima]

Attachment: Made all the required changes according to the comment 6

Please review this patch and let me know whether anymore changes are required.

Comment 8

[Madhurima]

Attachment: Made all the required changes according to the comment 6

Please review this patch and let me know whether anymore changes are required. Kindly excuse me for the previous comment.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8671539 [details] [diff] [review]
Made all the required changes according to the comment 6

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

Thanks for the fixups; just a couple more left.

I'm not entirely sure if the global substitutions of mozilla/Snprintf.h in place of prprf.h are necessarily correct; the files could use other functions declared by prprf.h.  But that's not something you need to worry about; I can take care of making sure the patch compiles everywhere, if you're not already verifying it compiles on your local machine.

::: netwerk/protocol/http/Http2Session.cpp
@@ +239,3 @@
>        line += 10;
>      }
> +    snprintf(line, 128, "%02X ", (reinterpret_cast<const uint8_t *>(data))[index]);

This converted |128 - (line - linebuf)| into a bare 128.  Please fix.

::: netwerk/protocol/http/SpdySession31.cpp
@@ +190,3 @@
>        line += 10;
>      }
> +    snprintf(line, 128, "%02X ", ((unsigned char *)data)[index]);

Likewise here.

Comment 10

[Madhurima]

Attachment: Made all the required changes according to the comment 9

Please review this patch and let me know whether anymore changes are required. Hope this one works.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8671840 [details] [diff] [review]
Made all the required changes according to the comment 9

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

Thanks for the patch, and for going through the revision process with me!  I'll take care of getting it landed, though it might not land until early next week.

Comment 12

[Nathan Froyd [:froydnj]]

Madhurima, your attachment 8671840 [details] [diff] [review] appears to not apply cleanly to mozilla-central.  Could you please generate a patch that applies cleanly to (an unpatched) mozilla-central?  Thanks!

Comment 13

[Madhurima]

Attachment: Made the required changes according to comment 12

Do let me know if this works.

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8680766 [details] [diff] [review]
Made the required changes according to comment 12

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

Thanks for fixing things up.  I'll take care of landing the patch!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8093d8be6a

Comment 16

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/59ca4989191f for Linux xpcshell failures in test_offlinecache_custom-directory.js like https://treeherder.mozilla.org/logviewer.html#?job_id=16616420&repo=mozilla-inbound

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3455c9aa3c

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9e3455c9aa3c

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-Central/rev/9e3455c9aa3c