Closed
Bug 1197311
Opened 9 years ago
Closed 8 years ago
remove PR_snprintf calls in dom/
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: froydnj, Assigned: sakshivaid95, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(1 file, 16 obsolete files)
63.22 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
Steps: 1. Find all calls to PR_snprintf in dom/. 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. It's worth noting that the calls in dom/system/gonk/NetworkUtils.cpp should be rewritten using snprintf_literal, rather than snprintf.
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to jmtrik from comment #1) > Am I OK to work on this as my first Bug? Yes!
Updated•9 years ago
|
Assignee: nobody → jmtrik
Attachment #8651955 -
Attachment description: <strike>1197311.patch</strike> → 1197311.patch
I have made some of the changes (see the attached patch). I have a few questions about the remaining files that I need to change. I have thes three files left to change. /dom/plugins/base/nsPluginHost.cpp /dom/media/webrtc/TRCCertificate.cpp /dom/system/gonk/NetworkUtils.cpp I will list the issues I have for each file. ----------- /dom/plugins/base/nsPluginHost.cpp The only occurance of PR_snprintf is on line 3771. I don't think we can change this as l != sizeof(p). Do you agree? - uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32; newBufferLen = dataLen + l; if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen))) return NS_ERROR_OUT_OF_MEMORY; headersLen = PR_snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF); - ---------- /dom/media/webrtc/TRCCertificate.cpp Similar story here. On line 98 we have - PR_snprintf(&buf[i * 2 + 3], 2, "%.2x", randomName[i]); - This should clearly not be changed. Again, do you agree? --------- /dom/system/gonk/NetworkUtils.cpp There seem to be inconsistencies with the value being passed as the second argument here. Is this for a good reason or can all of the occurances of lines like the following be changed PR_snprintf(key, PROPERTY_KEY_MAX - 1, ..... PR_snprintf(gCurrentCommand.command, MAX_COMMAND_SIZE - 1,... PR_snprintf(command, MAX_COMMAND_SIZE - 1,.... PR_snprintf(command, sizeof command,.... My worry is the last two seem to be inconsistent.
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to jmtrik from comment #5) > I have made some of the changes (see the attached patch). I have a few > questions about the remaining files that I need to change. Great! It's also worth noting that I forgot a step, which is to ensure the format characters correspond to the kind of data they're formatting. This is mostly ensuring that %ld, %lu, %lld, %llu, and similar get changed to use the standard formatting sequences. > /dom/plugins/base/nsPluginHost.cpp > > The only occurance of PR_snprintf is on line 3771. I don't think we can > change this as l != sizeof(p). Do you agree? You can change it to call snprintf instead of PR_snprintf. > /dom/media/webrtc/TRCCertificate.cpp > Similar story here. On line 98 we have > - > PR_snprintf(&buf[i * 2 + 3], 2, "%.2x", randomName[i]); > - > This should clearly not be changed. Again, do you agree? Please change it to call snprintf instead. > /dom/system/gonk/NetworkUtils.cpp > There seem to be inconsistencies with the value being passed as the second > argument here. Is this for a good reason or can all of the occurances of > lines like the following be changed > > PR_snprintf(key, PROPERTY_KEY_MAX - 1, ..... > PR_snprintf(gCurrentCommand.command, MAX_COMMAND_SIZE - 1,... > PR_snprintf(command, MAX_COMMAND_SIZE - 1,.... > PR_snprintf(command, sizeof command,.... You should be able to change all of these to call snprintf_literal. It's not clear to me why the -1 was subtracted in these cases, but it's not necessary.
OK thanks Nathan. That all makes sense. I'll finish making those changes.
Nathan, could you check this patch? Also, do you want strings like this changing to cross-platform macros? LOG("AppleVDADecoder[%s] status %d flags %d retainCount %ld",
Attachment #8651955 -
Attachment is obsolete: true
Reporter | ||
Comment 9•9 years ago
|
||
(In reply to jmtrik from comment #8) > Nathan, could you check this patch? I apologize for not responding to this! I'll look at the patch. > Also, do you want strings like this changing to cross-platform macros? > > LOG("AppleVDADecoder[%s] status %d flags %d retainCount %ld", I'm not sure what you mean here; could you elaborate?
Flags: needinfo?(jmtrik)
Comment 10•9 years ago
|
||
Made all the changes and fixed the indentation in the patch.
Assignee: jmtrik → allamsetty.anup
Attachment #8652050 -
Attachment is obsolete: true
Attachment #8688031 -
Flags: review?(nfroyd)
Reporter | ||
Comment 11•9 years ago
|
||
Comment on attachment 8688031 [details] [diff] [review] made the necessary changes as requested in the bug. Review of attachment 8688031 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing up the patch! I noted a number of issues below. I think all the MAX_COMMAND_SIZE - 1 snprintf calls could really be snprintf_literal, but I'm unsure of whether snprintf on Firefox OS would do the correct thing there. ::: dom/base/nsDocument.cpp @@ +2944,5 @@ > PRExplodedTime prtime; > PR_ExplodeTime(aTime, PR_LocalTimeParameters, &prtime); > // "MM/DD/YYYY hh:mm:ss" > char formatedTime[24]; > + if (snprintf_literal(formatedTime, "%02ld/%02ld/%04hd %02ld:%02ld:%02ld", The "%ld" format strings should be changed to "%d", and the "%hd" format strings should be changed to "%d" with the corresponding field (prtime.tm_year) changed to be cast to |int|. ::: dom/base/nsGlobalWindow.cpp @@ +1770,5 @@ > nsAutoCString uri; > if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) { > tmp->mDoc->GetDocumentURI()->GetSpec(uri); > } > + snprintf_literal(name, "nsGlobalWindow #%llu %s %s", tmp->mWindowID, The %llu here should be changed to use "%" PRIu64. ::: dom/base/nsXMLContentSerializer.cpp @@ +604,5 @@ > nsXMLContentSerializer::GenerateNewPrefix(nsAString& aPrefix) > { > aPrefix.Assign('a'); > char buf[128]; > + snprintf_literal(buf,, "%d", mPrefixIndex++); The argument list here contains an extra comma. ::: dom/events/DOMEventTargetHelper.cpp @@ +30,5 @@ > nsAutoString uri; > if (tmp->mOwnerWindow && tmp->mOwnerWindow->GetExtantDoc()) { > tmp->mOwnerWindow->GetExtantDoc()->GetDocumentURI(uri); > } > + snprintf(name, "DOMEventTargetHelper %s", This should be snprintf_literal. ::: dom/media/MediaManager.cpp @@ +2507,5 @@ > uint64_t outerID = outer->WindowID(); > > // Notify the UI that this window no longer has gUM active > char windowBuffer[32]; > + snprintf_literal(windowBuffer, "%llu", outerID); The format string here should use "%" PRIu64. ::: dom/plugins/base/nsPluginHost.cpp @@ +3758,5 @@ > uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32; > newBufferLen = dataLen + l; > if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen))) > return NS_ERROR_OUT_OF_MEMORY; > + headersLen = snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF); The "%ld" in this format string should be "%d". ::: dom/system/gonk/NetworkUtils.cpp @@ +652,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "nat disable %s %s 0", Nit: Please don't add trailing whitespace. @@ +663,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "nat enable %s %s 0", Nit: Please don't add trailing whitespace. @@ +749,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, Nit: please don't add trailing whitespace. @@ +761,5 @@ > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > > + snprintf(command, MAX_COMMAND_SIZE - 1, Nit: please don't add trailing whitespace. ::: dom/wifi/WifiUtils.cpp @@ +367,5 @@ > int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) { > char command[COMMAND_SIZE]; > if (!strcmp(iface, "p2p0")) { > // Commands for p2p0 interface don't need prefix > + snprintf(command, COMMAND_SIZE, "%s", cmd); This could be snprintf_literal. @@ +372,3 @@ > } > else { > + snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd); This could be snprintf_literal.
Attachment #8688031 -
Flags: review?(nfroyd) → feedback+
Comment 12•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #11) > Comment on attachment 8688031 [details] [diff] [review] > made the necessary changes as requested in the bug. > > Review of attachment 8688031 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for fixing up the patch! I noted a number of issues below. Okay, I will fix the issues and will update the patch. > > I think all the MAX_COMMAND_SIZE - 1 snprintf calls could really be > snprintf_literal, but I'm unsure of whether snprintf on Firefox OS would do > the correct thing there. Should I "NEEDINFO dohlbert" here for this?
Comment 13•9 years ago
|
||
Made all the changes as requested in comment 11 and updated the patch. Should we needinfo dohlbert here for this bug? because he might be helpful for the doubts related to snprintf_literal
Attachment #8688031 -
Attachment is obsolete: true
Attachment #8688270 -
Flags: review?(nfroyd)
Attachment #8688270 -
Flags: feedback?(nfroyd)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8688270 [details] [diff] [review] updated patch Review of attachment 8688270 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. For future patches you only need to set the review flag; I typically set the feedback flag to f+ to indicate that I think the patch is going in the correct general direction, even though it doesn't pass review. ::: dom/base/nsGlobalWindow.cpp @@ +1770,5 @@ > nsAutoCString uri; > if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) { > tmp->mDoc->GetDocumentURI()->GetSpec(uri); > } > + snprintf_literal(name, "nsGlobalWindow #"%" PRIu64 %s %s", tmp->mWindowID, This format string doesn't compile because (writing some spaces in here): "nsGlobalWindow #" % " PRIu64 %s %s" is trying to apply the |%| operator to two strings, which isn't going to work. It also looks like this file may need to include mozilla/IntegerPrintfMacros.h. ::: dom/wifi/WifiUtils.cpp @@ +372,3 @@ > } > else { > + snprintf_literal(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd); These snprintf_literal calls can't be correct, because COMMAND_SIZE isn't a format string. Please fix them.
Attachment #8688270 -
Flags: review?(nfroyd)
Attachment #8688270 -
Flags: feedback?(nfroyd)
Attachment #8688270 -
Flags: feedback+
Comment 15•8 years ago
|
||
Hi, I have followed all instructions provided and I am receiving the following errors provided in the attachment. Can you please give me an idea of why this is happening. Thanks
Comment 16•8 years ago
|
||
(In reply to Salah from comment #15) > Created attachment 8690422 [details] > Bug Error > > Hi, > I have followed all instructions provided and I am receiving the following > errors provided in the attachment. Can you please give me an idea of why > this is happening. Thanks Hi Salah, I am working on this bug, will be updating the patch soon, and will fix all the errors.
Comment 17•8 years ago
|
||
I have updated the patch as you requested in comment 14, but still the build is failing, don't know what the problem is. Can you please help me out?
Attachment #8688270 -
Attachment is obsolete: true
Attachment #8690422 -
Attachment is obsolete: true
Attachment #8690537 -
Flags: review?(nfroyd)
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8690537 [details] [diff] [review] bug1197311.patch (In reply to Anup Kumar from comment #17) > I have updated the patch as you requested in comment 14, but still the build > is failing, don't know what the problem is. > > Can you please help me out? It's difficult to help you figure out what the problem is if you don't provide details of what the build failure is.
Attachment #8690537 -
Flags: review?(nfroyd)
Comment 19•8 years ago
|
||
Here is the build error as you requested. It is producing the error at "snprintf_literal(name, "nsGlobalWindow #"%"PRIu64 %s %s", tmp->mWindowID,"
Attachment #8690666 -
Flags: review?(nfroyd)
Comment 20•8 years ago
|
||
Build error with the patch "bug1197311.patch" applied.
Attachment #8690666 -
Attachment is obsolete: true
Attachment #8690666 -
Flags: review?(nfroyd)
Reporter | ||
Comment 21•8 years ago
|
||
(In reply to Anup Kumar from comment #19) > Created attachment 8690666 [details] > build error while trying to run with the latest patch applied > > Here is the build error as you requested. > > It is producing the error at "snprintf_literal(name, "nsGlobalWindow > #"%"PRIu64 %s %s", tmp->mWindowID," I addressed this in comment 14. The '%' character should be inside a string, not outside it. Your other build error: 3:15.88 /home/reznord/mozilla/dom/media/MediaManager.cpp:2508:34: warning: invalid suffix on literal; C++11 requires a space between literal and identifier [-Wliteral-suffix] 3:15.88 snprintf_literal(windowBuffer, "%"PRIu64, outerID); means that you need to insert a space between |PRIu64| and any strings you're concatenating it with: |"%" PRIu64|, and you'll likely need to do the same for the nsGlobalWindow.cpp modification, too.
Comment 22•8 years ago
|
||
Fixed all the errors from the previous comments and tested the patch in my local tree by building it. The build is successful for me. Please tell me if any more changes are requires for this patch.
Attachment #8690537 -
Attachment is obsolete: true
Attachment #8690668 -
Attachment is obsolete: true
Flags: needinfo?(jmtrik)
Attachment #8691762 -
Flags: review?(nfroyd)
Reporter | ||
Comment 23•8 years ago
|
||
Comment on attachment 8691762 [details] [diff] [review] Fixed all the errors in dom/ Review of attachment 8691762 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing the build errors, though I'm a bit curious why the compiler didn't complain about the snprintf_literal calls pointed out below. ::: dom/base/nsGlobalWindow.cpp @@ +1771,5 @@ > nsAutoCString uri; > if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) { > tmp->mDoc->GetDocumentURI()->GetSpec(uri); > } > + snprintf_literal(name, "nsGlobalWindow # %PRIu64 %s %s", tmp->mWindowID, This compiles, but it doesn't do the right thing at runtime, since the format directive is now %P... ::: dom/system/gonk/NetworkUtils.cpp @@ +372,5 @@ > */ > static void getIFProperties(const char* ifname, IFProperties& prop) > { > char key[PROPERTY_KEY_MAX]; > + snprintf_literal(key, PROPERTY_KEY_MAX - 1, "net.%s.gw", ifname); This snprintf_literal call should be calling snprintf instead. There are a lot of calls like this in the remainder of the patch; I'm not going to point out each and every one. Please fix them all.
Attachment #8691762 -
Flags: review?(nfroyd) → feedback+
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 24•8 years ago
|
||
Made the changes requested in comment 23.
Attachment #8691762 -
Attachment is obsolete: true
Attachment #8703581 -
Flags: review?(nfroyd)
Reporter | ||
Comment 25•8 years ago
|
||
Comment on attachment 8703581 [details] [diff] [review] Fixed all the errors in dom/ Review of attachment 8703581 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the updates. Still some minor things to fix, and one or two significant ones, which I'll ni? folks about. ::: dom/base/nsGlobalWindow.cpp @@ +19,5 @@ > #include "nsIDOMStorageManager.h" > #include "mozilla/dom/DOMStorage.h" > #include "mozilla/dom/StorageEvent.h" > #include "mozilla/dom/StorageEventBinding.h" > +#include "mozilla/IntegerPrintfMacros.h"f I guess this doesn't compile? @@ +1787,5 @@ > nsAutoCString uri; > if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) { > tmp->mDoc->GetDocumentURI()->GetSpec(uri); > } > + snprintf_literal(name, "nsGlobalWindow # %" PRIu64" %s %s", tmp->mWindowID, Please insert a space between PRIu64 and the subsequent string. ::: dom/plugins/base/nsPluginHost.cpp @@ +3756,5 @@ > uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32; > newBufferLen = dataLen + l; > if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen))) > return NS_ERROR_OUT_OF_MEMORY; > + headersLen = snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF); Please change the %ld here to use %u. ::: dom/system/gonk/NetworkUtils.cpp @@ +713,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s %lld", GET_CHAR(mIfname), LLONG_MAX); Please change the %lld to use PRId64, and the LLONG_MAX to INT64_MAX. @@ +733,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld", GET_CHAR(mIfname), GET_FIELD(mThreshold)); Please change this %lld to use %d. It looks like mThreshold was silently changed in bug 1209654 to be a 64-bit integer without updating this format string, so I think changing the field access to int32_t(GET_FIELD(mThreshold)) is correct... @@ +754,5 @@ > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %ld", GET_FIELD(mThreshold)); Likewise here. @@ +2039,5 @@ > // Set default froute from system properties. > char key[PROPERTY_KEY_MAX]; > char gateway[PROPERTY_KEY_MAX]; > > + snprintf_literal(key, sizeof key - 1, "net.%s.gw", autoIfname.get()); This snprintf_literal call can't be correct, please fix it. ::: dom/wifi/WifiUtils.cpp @@ +368,5 @@ > int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) { > char command[COMMAND_SIZE]; > if (!strcmp(iface, "p2p0")) { > // Commands for p2p0 interface don't need prefix > + snprintf(command, COMMAND_SIZE, "%s", cmd); This could be snprintf_literal, yes? @@ +373,3 @@ > } > else { > + snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd); Same here.
Attachment #8703581 -
Flags: review?(nfroyd) → feedback+
Reporter | ||
Comment 26•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #25) > @@ +733,5 @@ > > CommandCallback aCallback, > > NetworkResultOptions& aResult) > > { > > char command[MAX_COMMAND_SIZE]; > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld", GET_CHAR(mIfname), GET_FIELD(mThreshold)); > > Please change this %lld to use %d. It looks like mThreshold was silently > changed in bug 1209654 to be a 64-bit integer without updating this format > string, so I think changing the field access to > int32_t(GET_FIELD(mThreshold)) is correct... > > @@ +754,5 @@ > > NetworkResultOptions& aResult) > > { > > char command[MAX_COMMAND_SIZE]; > > > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %ld", GET_FIELD(mThreshold)); > > Likewise here. Albert, Ethan, your names are associated with bug 1177236, which added these particular commands. What's the right resolution now that mThreshold is a 64-bit integer?
Flags: needinfo?(ettseng)
Flags: needinfo?(alberto.crespell)
Comment 27•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #25) > Comment on attachment 8703581 [details] [diff] [review] > Fixed all the errors in dom/ > > Review of attachment 8703581 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the updates. Still some minor things to fix, and one or two > significant ones, which I'll ni? folks about. > > ::: dom/base/nsGlobalWindow.cpp > @@ +19,5 @@ > > #include "nsIDOMStorageManager.h" > > #include "mozilla/dom/DOMStorage.h" > > #include "mozilla/dom/StorageEvent.h" > > #include "mozilla/dom/StorageEventBinding.h" > > +#include "mozilla/IntegerPrintfMacros.h"f > > I guess this doesn't compile? > > @@ +1787,5 @@ > > nsAutoCString uri; > > if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) { > > tmp->mDoc->GetDocumentURI()->GetSpec(uri); > > } > > + snprintf_literal(name, "nsGlobalWindow # %" PRIu64" %s %s", tmp->mWindowID, > > Please insert a space between PRIu64 and the subsequent string. > > ::: dom/plugins/base/nsPluginHost.cpp > @@ +3756,5 @@ > > uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32; > > newBufferLen = dataLen + l; > > if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen))) > > return NS_ERROR_OUT_OF_MEMORY; > > + headersLen = snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF); > > Please change the %ld here to use %u. > > ::: dom/system/gonk/NetworkUtils.cpp > @@ +713,5 @@ > > CommandCallback aCallback, > > NetworkResultOptions& aResult) > > { > > char command[MAX_COMMAND_SIZE]; > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s %lld", GET_CHAR(mIfname), LLONG_MAX); > > Please change the %lld to use PRId64, and the LLONG_MAX to INT64_MAX. > > @@ +733,5 @@ > > CommandCallback aCallback, > > NetworkResultOptions& aResult) > > { > > char command[MAX_COMMAND_SIZE]; > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld", GET_CHAR(mIfname), GET_FIELD(mThreshold)); > > Please change this %lld to use %d. It looks like mThreshold was silently > changed in bug 1209654 to be a 64-bit integer without updating this format > string, so I think changing the field access to > int32_t(GET_FIELD(mThreshold)) is correct... > > @@ +754,5 @@ > > NetworkResultOptions& aResult) > > { > > char command[MAX_COMMAND_SIZE]; > > > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %ld", GET_FIELD(mThreshold)); > > Likewise here. > > @@ +2039,5 @@ > > // Set default froute from system properties. > > char key[PROPERTY_KEY_MAX]; > > char gateway[PROPERTY_KEY_MAX]; > > > > + snprintf_literal(key, sizeof key - 1, "net.%s.gw", autoIfname.get()); > > This snprintf_literal call can't be correct, please fix it. > > ::: dom/wifi/WifiUtils.cpp > @@ +368,5 @@ > > int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) { > > char command[COMMAND_SIZE]; > > if (!strcmp(iface, "p2p0")) { > > // Commands for p2p0 interface don't need prefix > > + snprintf(command, COMMAND_SIZE, "%s", cmd); > > This could be snprintf_literal, yes? I don't think so. I think this could be |snprintf| itself. If this call is to be |snprintf_literal|, then all the remaining calls which are of the same format must also use |snprintf|, which would be against the comment 23. > > @@ +373,3 @@ > > } > > else { > > + snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd); > > Same here.
Flags: needinfo?(nfroyd)
Comment 28•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #26) > Albert, Ethan, your names are associated with bug 1177236, which added these > particular commands. What's the right resolution now that mThreshold is a > 64-bit integer? Yes. mThreshold is defined as long long integer now. https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkUtils.h#194 It was a long integer initially but caused an overflow problem, which was resolved by bug 1209654. But the patch missed one place to modify, which was addressed by bug 1225549 (and one more missing point is addressed in this bug). We should use %lld for mThreshold instead of %ld.
Flags: needinfo?(ettseng)
Flags: needinfo?(alberto.crespell)
Reporter | ||
Comment 29•8 years ago
|
||
(In reply to Anup Kumar [:Anupkumar] from comment #27) > (In reply to Nathan Froyd [:froydnj] from comment #25) > > ::: dom/wifi/WifiUtils.cpp > > @@ +368,5 @@ > > > int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) { > > > char command[COMMAND_SIZE]; > > > if (!strcmp(iface, "p2p0")) { > > > // Commands for p2p0 interface don't need prefix > > > + snprintf(command, COMMAND_SIZE, "%s", cmd); > > > > This could be snprintf_literal, yes? > > I don't think so. I think this could be |snprintf| itself. If this call is > to be |snprintf_literal|, then all the remaining calls which are of the same > format must also use |snprintf|, which would be against the comment 23. No. The problem that comment 23 was pointing out was that: snprintf_literal(key, PROPERTY_KEY_MAX - 1, "net.%s.gw", ifname); doesn't compile, and assuming you want to keep the PROPERTY_KEY_MAX - 1, it should be: snprintf(key, PROPERTY_KEY_MAX - 1, "net.%s.gw", ifname); Alternatively, as comment 6 suggests, you could make this: snprintf_literal(key, "net.%s.gw", ifname); So the review comment stands, and really meant that you could write: snprintf_literal(command, "%s", cmd);
Flags: needinfo?(nfroyd)
Comment 30•8 years ago
|
||
made the changes as requested in above comments.
Attachment #8703581 -
Attachment is obsolete: true
Attachment #8704729 -
Flags: review?(nfroyd)
Comment 31•8 years ago
|
||
Comment on attachment 8704729 [details] [diff] [review] fixed all the errors Review of attachment 8704729 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/NetworkUtils.cpp @@ +734,5 @@ > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %d", > + GET_CHAR(mIfname), int32_t(GET_FIELD(mThreshold))); mThreshold is long long. This should be %lld. @@ +755,5 @@ > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %d", int32_t(GET_FIELD(mThreshold))); Same here.
Reporter | ||
Comment 32•8 years ago
|
||
Comment on attachment 8704729 [details] [diff] [review] fixed all the errors Review of attachment 8704729 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing things up. A few more comments, and please address Ethan's comments as well. ::: dom/base/nsGlobalWindow.cpp @@ +1787,5 @@ > nsAutoCString uri; > if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) { > tmp->mDoc->GetDocumentURI()->GetSpec(uri); > } > + snprintf_literal(name, "nsGlobalWindow # % " PRIu64" %s %s", tmp->mWindowID, You missed the space insertion requested in comment 28. ::: dom/system/gonk/NetworkUtils.cpp @@ +713,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s % PRId64", GET_CHAR(mIfname), INT64_MAX); Please insert a space between PRId64 and the following '"' character. ::: dom/wifi/WifiUtils.cpp @@ +368,5 @@ > int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) { > char command[COMMAND_SIZE]; > if (!strcmp(iface, "p2p0")) { > // Commands for p2p0 interface don't need prefix > + snprintf_literal(command, COMMAND_SIZE, "%s", cmd); This snprintf_literal call doesn't compile. @@ +373,3 @@ > } > else { > + snprintf_literal(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd); Neither does this one.
Attachment #8704729 -
Flags: review?(nfroyd) → feedback+
Comment 33•8 years ago
|
||
Re-assigning the bug to another contributor since I am getting busy for a while. (don't want the bug to be staged)
Assignee: allamsetty.anup → sakshivaid95
Comment 34•8 years ago
|
||
Comment on attachment 8704729 [details] [diff] [review] fixed all the errors Review of attachment 8704729 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webm/WebMDemuxer.cpp @@ +121,1 @@ > PR_vsnprintf(msg+strlen(msg), sizeof(msg)-strlen(msg), aFormat, args); This doesn't compile since we are removing "prprf.h" from this file. I crossed checked it with the "mozilla/Snprintf.h" header file and there |vsnprintf()| is declared. So, I think this needs to be changed to |vsnprintf()|. What do you say froydnj?
Attachment #8704729 -
Flags: feedback+ → feedback?(nfroyd)
Reporter | ||
Comment 35•8 years ago
|
||
(In reply to Anup Kumar [:Anupkumar] from comment #34) > ::: dom/media/webm/WebMDemuxer.cpp > @@ +121,1 @@ > > PR_vsnprintf(msg+strlen(msg), sizeof(msg)-strlen(msg), aFormat, args); > > This doesn't compile since we are removing "prprf.h" from this file. I > crossed checked it with the "mozilla/Snprintf.h" header file and there > |vsnprintf()| is declared. > > So, I think this needs to be changed to |vsnprintf()|. > > What do you say froydnj? I think the right thing here is to leave the prprf.h include and leave the PR_vsnprintf call unchanged; according to the comment in Snprintf.h, we can't use vsnprintf on Windows due to an MSVC bug.
Reporter | ||
Updated•8 years ago
|
Attachment #8704729 -
Flags: feedback?(nfroyd) → feedback+
Assignee | ||
Comment 36•8 years ago
|
||
Please review.
Attachment #8707523 -
Flags: review?(nfroyd)
Attachment #8707523 -
Flags: review?(allamsetty.anup)
Comment 37•8 years ago
|
||
Comment on attachment 8707523 [details] [diff] [review] Made changes as per the previous comments. bug1197311.patch Review of attachment 8707523 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #8707523 -
Flags: review?(allamsetty.anup) → review+
Reporter | ||
Comment 38•8 years ago
|
||
Comment on attachment 8707523 [details] [diff] [review] Made changes as per the previous comments. bug1197311.patch Review of attachment 8707523 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing things up! I noticed a few changes that hadn't been made, as well as some things I missed earlier. ::: dom/base/nsGlobalWindow.cpp @@ +1785,5 @@ > nsAutoCString uri; > if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) { > tmp->mDoc->GetDocumentURI()->GetSpec(uri); > } > + snprintf_literal(name, "nsGlobalWindow # % " PRIu64 " %s %s", tmp->mWindowID, Sorry for missing this in the last review, but you don't want any space between the first '%' and the closing '"'; the way it's written now, snprintf won't work correctly because "% " isn't a format directive. ::: dom/system/gonk/NetworkUtils.cpp @@ +734,5 @@ > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld", > + GET_CHAR(mIfname), int32_t(GET_FIELD(mThreshold))); Per Ethan's comment 28, please remove the int32_t cast. @@ +755,5 @@ > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %lld", int32_t(GET_FIELD(mThreshold))); Likewise here. @@ +1181,5 @@ > > for (uint32_t i = 0; i < length; i++) { > NS_ConvertUTF16toUTF8 autoDns(dnses[i]); > > + int ret = snprintf_literal(command + written, sizeof(command) - written, " %s", autoDns.get()); Sorry for not catching this earlier; this should be an snprintf call, not an snprintf_literal call. ::: dom/wifi/WifiUtils.cpp @@ +368,5 @@ > int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) { > char command[COMMAND_SIZE]; > if (!strcmp(iface, "p2p0")) { > // Commands for p2p0 interface don't need prefix > + snprintf(command, COMMAND_SIZE, "%s", cmd); Per comment 29, please change this to use snprintf_literal instead. Remember to remove the COMMAND_SIZE argument. @@ +373,3 @@ > } > else { > + snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd); Likewise this as well.
Attachment #8707523 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 39•8 years ago
|
||
Please review.
Attachment #8707523 -
Attachment is obsolete: true
Attachment #8708246 -
Flags: review?(nfroyd)
Attachment #8708246 -
Flags: review?(allamsetty.anup)
Reporter | ||
Comment 40•8 years ago
|
||
Comment on attachment 8708246 [details] [diff] [review] Made changes according to the comment 38 Review of attachment 8708246 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you!
Attachment #8708246 -
Flags: review?(nfroyd)
Attachment #8708246 -
Flags: review?(allamsetty.anup)
Attachment #8708246 -
Flags: review+
Assignee | ||
Comment 41•8 years ago
|
||
Thank you :)
Reporter | ||
Comment 42•8 years ago
|
||
Your most recent patch doesn't seem to apply cleanly to current m-c. Could you please rebase it and upload a new version? Thanks!
Flags: needinfo?(sakshivaid95)
Assignee | ||
Comment 43•8 years ago
|
||
Changes made as per comment 42. Please review.
Attachment #8708246 -
Attachment is obsolete: true
Flags: needinfo?(sakshivaid95)
Attachment #8713698 -
Flags: review?(nfroyd)
Attachment #8713698 -
Flags: review?(allamsetty.anup)
Reporter | ||
Comment 44•8 years ago
|
||
Comment on attachment 8713698 [details] [diff] [review] Removed PR_snprintf calls in dom/ Review of attachment 8713698 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating this! It applies just fine locally, but a try push reveals a few problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e9ad55c7bf&exclusion_profile=false ::: dom/system/gonk/NetworkUtils.cpp @@ +378,2 @@ > Property::Get(key, prop.gateway, ""); > PR_snprintf(key, Property::KEY_MAX_LENGTH - 1, "net.%s.dns1", ifname); Looks like you should fix this... @@ +378,4 @@ > Property::Get(key, prop.gateway, ""); > PR_snprintf(key, Property::KEY_MAX_LENGTH - 1, "net.%s.dns1", ifname); > Property::Get(key, prop.dns1, ""); > PR_snprintf(key, Property::KEY_MAX_LENGTH - 1, "net.%s.dns2", ifname); ...and this, too. Not sure how those snuck through all this time! @@ +714,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s % PRId64 ", GET_CHAR(mIfname), INT64_MAX); This bit of "% PRId64 " doesn't look right. @@ +1444,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "interface setmtu %s %d", The compiler says this %d should be %ld instead. @@ +1833,5 @@ > for (uint32_t i = 0; i < length; i++) { > NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]); > > char dns_prop_key[Property::VALUE_MAX_LENGTH]; > + snprintf(dns_prop_key, sizeof dns_prop_key, "net.dns%d", i+1); There's a PR_snprintf call just below here that needs changing. Also, this call can use snprintf_literal.
Attachment #8713698 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 45•8 years ago
|
||
Please review.
Attachment #8713698 -
Attachment is obsolete: true
Attachment #8713698 -
Flags: review?(allamsetty.anup)
Attachment #8714000 -
Flags: review?(nfroyd)
Reporter | ||
Updated•8 years ago
|
Attachment #8714000 -
Attachment is patch: true
Reporter | ||
Comment 46•8 years ago
|
||
Comment on attachment 8714000 [details] [diff] [review] Made changes as per comment 44. Review of attachment 8714000 [details] [diff] [review]: ----------------------------------------------------------------- In addition to fixing the bits below, please fix the commit message so that it just reads: Bug 1197311 - remove PR_snprintf calls in dom/ r=froydnj There's no need to have it say the same thing three different times. ::: dom/system/gonk/NetworkUtils.cpp @@ +714,5 @@ > CommandCallback aCallback, > NetworkResultOptions& aResult) > { > char command[MAX_COMMAND_SIZE]; > + snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s % PRId64", GET_CHAR(mIfname), INT64_MAX); Did you forget to fix the PRId64 bit here? @@ +1833,5 @@ > for (uint32_t i = 0; i < length; i++) { > NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]); > > char dns_prop_key[Property::VALUE_MAX_LENGTH]; > + snprintf_literal(dns_prop_key, sizeof dns_prop_key, "net.dns%d", i+1); When I said "this call can use snprintf_literal", I meant to change the call and to change the arguments, since snprintf_literal and snprintf have different argument lists. Please fix this.
Attachment #8714000 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 47•8 years ago
|
||
Please review.
Attachment #8714000 -
Attachment is obsolete: true
Attachment #8715366 -
Flags: review?(nfroyd)
Reporter | ||
Comment 48•8 years ago
|
||
Comment on attachment 8715366 [details] [diff] [review] Made changes as per comment 46. Review of attachment 8715366 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good, thank you! But it also needs an update to apply cleanly to current mozilla-central. Could you do that, please?
Attachment #8715366 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 49•8 years ago
|
||
Sorry for the late reply. Please review.
Attachment #8715366 -
Attachment is obsolete: true
Attachment #8719839 -
Flags: review?(nfroyd)
Reporter | ||
Updated•8 years ago
|
Attachment #8704729 -
Attachment is obsolete: true
Reporter | ||
Comment 50•8 years ago
|
||
Comment on attachment 8719839 [details] [diff] [review] Made changes as per the previous comment. Review of attachment 8719839 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating it. A try run looks like it's in good shape.
Attachment #8719839 -
Flags: review?(nfroyd) → review+
Comment 52•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/deadb414ee23
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1369543
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•