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

RESOLVED FIXED in Firefox 52

Status

()

enhancement
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: wes, Assigned: tromey)

Tracking

1.9.2 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(8 attachments, 5 obsolete attachments)

58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
evilpie
: review+
Details
58 bytes, text/x-review-board-request
evilpie
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
evilpie
: review+
Details
58 bytes, text/x-review-board-request
evilpie
: review+
Details
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
Duplicate of this bug: 1002852
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
Duplicate of this bug: 1308964
Depends on: 1331349
Duplicate of this bug: 1306537
You need to log in before you can comment on or make changes to this bug.