Closed Bug 553032 Opened 15 years ago Closed 8 years ago

JSAPI should use __attribute__((printf,x,y)) where possible

Categories

(Core :: JavaScript Engine, enhancement)

Product:
Core
Component:
JavaScript Engine
Version:
1.9.2 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: wes, Assigned: tromey)

References

Details

Attachments

(8 files, 5 obsolete files)

Bug 553032 - move MOZ_FORMAT_PRINTF to mfbt;
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 553032 - use MOZ_FORMAT_PRINTF, not explicit attribute;
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 553032 - use MOZ_FORMAT_PRINTF in js;
58 bytes, text/x-review-board-request
evilpie
: review+
Details
Bug 553032 - change sprintf_append to be a varargs function;
58 bytes, text/x-review-board-request
evilpie
: review+
Details
Bug 553032 - document MOZ_FORMAT_PRINTF;
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 553032 - add unit tests for JS_smprintf;
58 bytes, text/x-review-board-request
evilpie
: review+
Details
Bug 553032 - use PRIuSIZE rather than %z in js;
58 bytes, text/x-review-board-request
evilpie
: review+
Details
Bug 553032 - printf fix in MStoreSlot::printOpcode;
58 bytes, text/x-review-board-request
h4writer
: review+
Details
This GCC attribute helps to validate printf format strings against the supplied arguments. Adding this attribute in header files wherever possible will help to reduce code defects by adding a teensy-tiny layer of static analysis to both internal code, and JSAPI embeddings. Immediate candidates I spot in jsapi.h: - JS_ReportError, - JS_ReportWarning, - probably more if we get looking, possibly in NSPR too Routines like JS_ReportErrorNumberUC() *might* be able to benefit if the implementation changes via a macro so that errorNumber somehow winds up being a compile-time array lookup.
Assignee: general → nobody
I looked into this a bit as a side excursion from bug 987069. One thing worth noting is that functions like JS_ReportError call JS_vsmprintf (or similar), and these accept extensions to the ordinary set of printf specifiers -- in particular "%hs" for char16_t*. So, some of these functions can't be annotated without a gcc extension; since at present there is no standard format specifier for char16_t.
I found a number of other spots that could use this treatment as well. However so far I've only done js. I'll attach the patch momentarily. So far I've only built it on linux.
There was some pre-existing support for these attributes, just buried in xpcom. This moves MOZ_FORMAT_PRINTF to mfbt and adds MOZ_FORMAT_SCANF.
This adds MOZ_FORMAT_PRINTF markers to js and then fixes up the fallout. There were many errors, though most of a trivial nature. However there were some cases that were clearly just wrong (search for "asInt" in the patch).
TIL about bug 1060419, which covers this for parts of xpcom. However there are still more cases that benefit.
(In reply to Tom Tromey :tromey from comment #1) > One thing worth noting is that functions like JS_ReportError call > JS_vsmprintf (or similar), and these accept extensions to the ordinary > set of printf specifiers -- in particular "%hs" for char16_t*. Can we not use %ls for these? That's for wchar_t, but both MSVC 2013 and GCC appear to support it [1] [2] (which I assume means clang does as well). [1] https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx [2] http://www.gnu.org/software/libc/manual/html_node/Other-Output-Conversions.html
Frankly, variadic functions and format specifiers are an anti-pattern. I don't think we should be improving any of it, and we should be striving to remove it all. And changing %hs to %ls is, honestly, just horribly mean to embedders. I argue we should WONTFIX this entire bug.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #6) > (In reply to Tom Tromey :tromey from comment #1) > > One thing worth noting is that functions like JS_ReportError call > > JS_vsmprintf (or similar), and these accept extensions to the ordinary > > set of printf specifiers -- in particular "%hs" for char16_t*. > > Can we not use %ls for these? That's for wchar_t, but both MSVC 2013 and GCC > appear to support it [1] [2] (which I assume means clang does as well). No, because wchar_t and char16_t aren't necessarily the same, and the format extension exists in the js printf-alike in order to print char16_t*. E.g., on Linux wchar_t is actually UCS-4.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7) > Frankly, variadic functions and format specifiers are an anti-pattern. I > don't think we should be improving any of it, and we should be striving to > remove it all. And changing %hs to %ls is, honestly, just horribly mean to > embedders. I argue we should WONTFIX this entire bug. I think removing printf-likes ought to be a precondition to WONTFIXing this bug. Otherwise it seems like we're just burying our heads in the sand. The current series I have finds some bugs in the current code -- not just size differences but a few spots where we're passing an aggregate to printf. It's mostly logging code, so perhaps the exposure is not too bad.
(In reply to Tom Tromey :tromey from comment #9) > Otherwise it seems like we're just burying our heads in the sand. Guilty as charged. We don't often go out of our way to do API-level wide-ranging changes unless there's some other prior justification for it. This is indeed mostly logging code, or slightly-wrong error messages and such, so usually doesn't rise quite high enough to justify going out of the way to remove it immediately. Worth not adding new uses, perhaps worth designing the replacement API and starting to use it, yeah. But not worth improving (which we can't even do, because of %hs and similar) just because we're not quite ready to invest the effort to remove it.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > (In reply to Tom Tromey :tromey from comment #9) > > Otherwise it seems like we're just burying our heads in the sand. > > Guilty as charged. > > We don't often go out of our way to do API-level wide-ranging changes unless > there's some other prior justification for it. This is indeed mostly > logging code, or slightly-wrong error messages and such, so usually doesn't > rise quite high enough to justify going out of the way to remove it > immediately. Worth not adding new uses, perhaps worth designing the > replacement API and starting to use it, yeah. But not worth improving > (which we can't even do, because of %hs and similar) just because we're not > quite ready to invest the effort to remove it. So FWIW the reason I got on this bug at all was trying to find the spots where a non-POD is passed to a varargs function in bug 987069. There, (1) clang doesn't report any errors at all, either on my Linux box nor on "try" (I realize it does for you, but my main interest is trying to prevent future regressions), and (2) gcc implements the C++11 trivially-copyable semantics change, so the warning is bypassed by the way ConstUTF8ZString is written (and in any case the warning is only available in a way that brings in other warnings that we don't want). However, the patch in this bug does at least successfully catch many of the errors. And, the extended series I have (which includes bug 1060419 plus some other things) catches more issues, including, I believe, some actual bugs in non-logging code. So to sum up, I see it as a reasonable safety precaution, which furthermore is rather cheap, both in terms of implementation and in cognitive load. It's true it doesn't work for all printf-alikes -- not just due to the %hs issue, but nsTextFormatter.h has some that use char16_t* for the format argument, which is also not checkable -- but here my view is that some checking is better than no checking. Also, I've filed bugs for all the issues with GCC, so there's some hope that a future GCC will let us do more.
(In reply to Tom Tromey :tromey from comment #8) > No, because wchar_t and char16_t aren't necessarily the same, [...] > E.g., on Linux wchar_t is actually UCS-4. Thanks, I had no idea.
Attachment #8556736 - Attachment is obsolete: true
Attachment #8556739 - Attachment is obsolete: true
I revisited this bug today. This time I took a new approach, namely changing the jsprf code to be more standards-compliant; then adding the attributes; then fixing all the uses. I think this is a good approach because right now js/ uses a mix of nspr-like and standard-like printf-like functions. This is confusing. While these patches won't go as far as bug 1300320, I think they still represent an improvement. They found a few small formatting bugs. This series also fixes bug 1002852 and bug 1308964.
One additional note is that, while I've split this up for easier review, the final few patches will have to be squashed in order to land.
Also perhaps I should write some unit tests for jsprf.
Attachment #8800366 - Flags: review?(evilpies) → review?(nfroyd)
Attachment #8800367 - Flags: review?(evilpies) → review?(nfroyd)
Comment on attachment 8800368 [details] Bug 553032 - use MOZ_FORMAT_PRINTF in js; https://reviewboard.mozilla.org/r/85274/#review84112
Attachment #8800368 - Flags: review?(evilpies) → review+
Comment on attachment 8800369 [details] Bug 553032 - change sprintf_append to be a varargs function; https://reviewboard.mozilla.org/r/85276/#review84114
Attachment #8800369 - Flags: review?(evilpies) → review+
Comment on attachment 8800370 [details] Bug 553032 - add -Wformat=error and fix up fallout; https://reviewboard.mozilla.org/r/85278/#review84122 I skimmed this, but because most of the time the type definitions are invisible this is hard to verify. I assume the compiler is correct here. ::: js/src/moz.build:784 (Diff revision 1) > else: > # Windows needs this to be linked with a static library. > DEFINES['FFI_BUILDING'] = True > > if CONFIG['GNU_CXX']: > - CXXFLAGS += ['-Wno-shadow'] > + CXXFLAGS += ['-Wno-shadow', '-Werror=format'] In general we need build peer reviews for build changes. ::: js/xpconnect/src/XPCThrower.cpp:121 (Diff revision 1) > format = ""; > > if (nsXPCException::NameAndFormatForNSResult(result, &name, nullptr) && name) > - sz = JS_smprintf("%s 0x%x (%s)", format, result, name); > + sz = JS_smprintf("%s 0x%x (%s)", format, (unsigned) result, name); > else > - sz = JS_smprintf("%s 0x%x", format, result); > + sz = JS_smprintf("%s 0x%" PRIu32 "x", format, (unsigned) result); I don't quite understand the difference between PRIx32 and PRIu32 "x". Should we use the former?
Comment on attachment 8800371 [details] Bug 553032 - convert jsprf functions to use standard escapes; https://reviewboard.mozilla.org/r/85280/#review84124 Can you clarify why you can't just rip out the old string formatting code and replace it with snprintf or similar? You are already using the gcc checks for those format anyway, so we should be correct in that regard. I guess one concern is nullptr handling?
(In reply to Tom Schuster [:evilpie] from comment #24) > I don't quite understand the difference between PRIx32 and PRIu32 "x". > Should we use the former? Nope, 'PRIu32 "x"' is a bug, sorry about that. I'll re-read the patch to try to find other such errors.
(In reply to Tom Schuster [:evilpie] from comment #25) > Comment on attachment 8800371 [details] > Bug 553032 - convert jsprf functions to use standard escapes; > > https://reviewboard.mozilla.org/r/85280/#review84124 > > Can you clarify why you can't just rip out the old string formatting code > and replace it with snprintf or similar? You are already using the gcc > checks for those format anyway, so we should be correct in that regard. I > guess one concern is nullptr handling? I had three reasons. 1. Replacing the JS functions with standard functions can be deferred, making these patches somewhat simpler to digest. In retrospect, given the last patch, I'm not sure that's really true. 2. The nullptr thing 3. Efficiency concerns when printing to memory. glibc provides asprintf, which solves this, but I don't think that's available elsewhere. I think the specific problem is that one can use vsnprintf but then the code has to grow the buffer and try again if the original guess was too small.
Comment on attachment 8800366 [details] Bug 553032 - move MOZ_FORMAT_PRINTF to mfbt; https://reviewboard.mozilla.org/r/85270/#review84192
Attachment #8800366 - Flags: review?(nfroyd) → review+
Comment on attachment 8800367 [details] Bug 553032 - use MOZ_FORMAT_PRINTF, not explicit attribute; https://reviewboard.mozilla.org/r/85272/#review84194
Attachment #8800367 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #28) > Comment on attachment 8800366 [details] > Bug 553032 - move MOZ_FORMAT_PRINTF to mfbt; > > https://reviewboard.mozilla.org/r/85270/#review84192 I forgot to mention that it would be most excellent if this attribute had some documentation associated with it in the header file, so that people don't have to go look things up in GCC documentation. That can be done here as a separate patch or a follow-up bug.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
(In reply to Tom Tromey :tromey from comment #26) > (In reply to Tom Schuster [:evilpie] from comment #24) > > > I don't quite understand the difference between PRIx32 and PRIu32 "x". > > Should we use the former? > > Nope, 'PRIu32 "x"' is a bug, sorry about that. > I'll re-read the patch to try to find other such errors. I found two. One should have remained %x and one I fixed by removing the "x".
Comment on attachment 8800370 [details] Bug 553032 - add -Wformat=error and fix up fallout; https://reviewboard.mozilla.org/r/85278/#review84486 ::: js/src/asmjs/AsmJS.cpp:4722 (Diff revision 2) > > static bool > CheckSignatureAgainstExisting(ModuleValidator& m, ParseNode* usepn, const Sig& sig, const Sig& existing) > { > if (sig.args().length() != existing.args().length()) { > - return m.failf(usepn, "incompatible number of arguments (%u here vs. %u before)", > + return m.failf(usepn, "incompatible number of arguments (%zu here vs. %zu before)", This should be PRIuSIZE. GCC/clang don't care that %zu doesn't work in MSVC.
Comment on attachment 8800370 [details] Bug 553032 - add -Wformat=error and fix up fallout; https://reviewboard.mozilla.org/r/85278/#review84488
Attachment #8800370 - Flags: review?(evilpies) → review+
Comment on attachment 8800371 [details] Bug 553032 - convert jsprf functions to use standard escapes; https://reviewboard.mozilla.org/r/85280/#review84490 It looks correct, but I really wish you had removed this code instead ...
Attachment #8800371 - Flags: review?(evilpies) → review+
Comment on attachment 8800845 [details] Bug 553032 - add unit tests for JS_smprintf; https://reviewboard.mozilla.org/r/85676/#review84492
Attachment #8800845 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #40) > This should be PRIuSIZE. GCC/clang don't care that %zu doesn't work in MSVC. Thanks. I've fixed this and I've also tacked on an additional patch to fix up all the pre-existing %z uses in js. Will attach soon.
Comment on attachment 8801108 [details] Bug 553032 - use PRIuSIZE rather than %z in js; https://reviewboard.mozilla.org/r/85902/#review84616 Thanks for checking that.
Attachment #8801108 - Flags: review?(evilpies) → review+
Comment on attachment 8800370 [details] Bug 553032 - add -Wformat=error and fix up fallout; This needs a build peer review, see comment #24.
Attachment #8800370 - Flags: review?(ted)
Comment on attachment 8800844 [details] Bug 553032 - document MOZ_FORMAT_PRINTF; https://reviewboard.mozilla.org/r/85674/#review84900 Thank you!
Attachment #8800844 - Flags: review?(nfroyd) → review+
The initial try build was terrible, partly due to this bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78014 I don't think this is a major concern for ordinary development, just for this initial cleanup.
Comment on attachment 8800370 [details] Bug 553032 - add -Wformat=error and fix up fallout; https://reviewboard.mozilla.org/r/85278/#review85214 ::: js/src/moz.build:785 (Diff revision 3) > else: > # Windows needs this to be linked with a static library. > DEFINES['FFI_BUILDING'] = True > > if CONFIG['GNU_CXX']: > - CXXFLAGS += ['-Wno-shadow'] > + CXXFLAGS += ['-Wno-shadow', '-Werror=format'] It would be nice to move these to somewhere like js/moz.configure at some point so we don't have to repeat them in every moz.build under js/src, but I don't know if we have an easy way to do that yet.
Attachment #8800370 - Flags: review?(ted) → review+
Building on try pointed out a few issues. I installed a 32-bit toolchain and libraries here to help catch them locally; and this worked ok. However, something is still different, because on try I see errors from -Wformat-security... which is great, it's an improvement, just puzzling since I don't see them here. The more ordinary fixes I will just roll into the earlier patches, but I will tack on a patch or two to address the -Wformat-security errors.
Comment on attachment 8802293 [details] Bug 553032 - fix calls to printf-likes for -Wformat-security; https://reviewboard.mozilla.org/r/86718/#review85676 ::: js/xpconnect/wrappers/AccessCheck.cpp:274 (Diff revision 1) > } > > enum Access { READ = (1<<0), WRITE = (1<<1), NO_ACCESS = 0 }; > > static void > -EnterAndThrowASCII(JSContext* cx, JSObject* wrapper, const char* msg) > +EnterAndThrowASCII(JSContext* cx, JSObject* wrapper, const char* msg, ...) Why did you make this a variable arg?
Attachment #8802293 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #65) > > -EnterAndThrowASCII(JSContext* cx, JSObject* wrapper, const char* msg) > > +EnterAndThrowASCII(JSContext* cx, JSObject* wrapper, const char* msg, ...) > > Why did you make this a variable arg? Thanks for catching that. That was my first thought and I didn't back it out sufficiently. I've removed it now. I'm going to push a new series with some patches squashed. This shouldn't result in new review requests, I hope.
Attachment #8800370 - Attachment is obsolete: true
Attachment #8802293 - Attachment is obsolete: true
Attachment #8800371 - Attachment is obsolete: true
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16f326945f38 move MOZ_FORMAT_PRINTF to mfbt; r=froydnj https://hg.mozilla.org/integration/autoland/rev/e832fc3b5a03 document MOZ_FORMAT_PRINTF; r=froydnj https://hg.mozilla.org/integration/autoland/rev/818fc5631d72 use MOZ_FORMAT_PRINTF, not explicit attribute; r=froydnj https://hg.mozilla.org/integration/autoland/rev/2bfd163f23f9 use MOZ_FORMAT_PRINTF in js; r=evilpie https://hg.mozilla.org/integration/autoland/rev/2129aca7cab2 change sprintf_append to be a varargs function; r=evilpie https://hg.mozilla.org/integration/autoland/rev/4d1e74123ab6 add unit tests for JS_smprintf; r=evilpie https://hg.mozilla.org/integration/autoland/rev/9055ad92499a use PRIuSIZE rather than %z in js; r=evilpie
I'll attach a fix for that shortly. The plan as discussed on irc is to autoland the patch and then I'll be available to fix any further issues of this kind while the patches are merged around.
Flags: needinfo?(ttromey)
Rebased, and needed one more little patch.
... and also added a few minor tests to the new test case, to be sure that PRI* works on all platforms.
Comment on attachment 8803002 [details] Bug 553032 - printf fix in MStoreSlot::printOpcode; https://reviewboard.mozilla.org/r/87244/#review86348
Attachment #8803002 - Flags: review?(hv1989) → review+
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ce2b6b8bfa16 move MOZ_FORMAT_PRINTF to mfbt; r=froydnj https://hg.mozilla.org/integration/autoland/rev/7ece94001875 document MOZ_FORMAT_PRINTF; r=froydnj https://hg.mozilla.org/integration/autoland/rev/65a272de7eda use MOZ_FORMAT_PRINTF, not explicit attribute; r=froydnj https://hg.mozilla.org/integration/autoland/rev/cf0ce58e21e1 printf fix in MStoreSlot::printOpcode; r=h4writer https://hg.mozilla.org/integration/autoland/rev/353578b40e7a use MOZ_FORMAT_PRINTF in js; r=evilpie https://hg.mozilla.org/integration/autoland/rev/f6c702f47a7e change sprintf_append to be a varargs function; r=evilpie https://hg.mozilla.org/integration/autoland/rev/482223f97550 add unit tests for JS_smprintf; r=evilpie https://hg.mozilla.org/integration/autoland/rev/a12133c22f12 use PRIuSIZE rather than %z in js; r=evilpie
Depends on: 1331349
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: