1121290 - use format specifiers correctly

Status: RESOLVED DUPLICATE of bug 1124033

(Firefox Build System :: General, defect)

Description

[[e-mail]]

Attachment: use ls instead s in format string

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150113122454

Steps to reproduce:

Specifiers s and ls are different argument types,  which are char* and wchar_t*. (http://en.cppreference.com/w/cpp/io/c/fprintf)
Prior to MSVC 2015, VC will accept s as wchar_t*. But with vc14, the code without the patch give wrong results. 
According to http://blogs.msdn.com/b/vcblog/archive/2014/11/12/improvements-to-warnings-in-the-c-compiler.aspx, vc14 rtm will "check the types of the parameters passed to these functions", so I think this change are intensional intended.

Brian Smith, can you file the bug at https://connect.microsoft.com?

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8548575 [details] [diff] [review]
use ls instead s in format string

Thanks for the explanation. So, basically, this is a bug in the Gecko code and you expect that the fixed version will work with older versions of MSVC and newer versions, right?

Comment 2

[[e-mail]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)
> Comment on attachment 8548575 [details] [diff] [review]
> use ls instead s in format string
> 
> Thanks for the explanation. So, basically, this is a bug in the Gecko code
> and you expect that the fixed version will work with older versions of MSVC
> and newer versions, right?

Yes.

Comment 3

[Benjamin Smedberg]

Comment on attachment 8548575 [details] [diff] [review]
use ls instead s in format string

I don't think this is correct. According to the docs:

`s` When used with printf functions, specifies a single-byte or multi-byte character string; when used with wprintf functions, specifies a wide-character string.

Since this is used with _snwprintf_s, and `key` is a wide string, the current formatting is correct.

%ls is for use with narrow (char*) strings, and there are none of these in this code.

Comment 4

[Benjamin Smedberg]

I'm going to set NEEDINFO on this before closing it, though, since perhaps I missed something.

Comment 5

[[e-mail]]

I am familiar with the details. But the original code will give wrong results with vc14.

check the results on http://webcompiler.cloudapp.net/
wprintf(L"1 %s\n","some string"); //Good
wprintf(L"2 %s\n",L"some string"); //Not good -> print only first character of the string
printf("3 %s\n","some string"); //Good
printf("4 %s\n",L"some string"); //Not good -> print only first character of the string

Comment 6

[[e-mail]]

(In reply to zhoubcfan from comment #5)
> I am familiar with the details. But the original code will give wrong
> results with vc14.
> 
> check the results on http://webcompiler.cloudapp.net/
> wprintf(L"1 %s\n","some string"); //Good
> wprintf(L"2 %s\n",L"some string"); //Not good -> print only first character
> of the string
> printf("3 %s\n","some string"); //Good
> printf("4 %s\n",L"some string"); //Not good -> print only first character of
> the string

not familiar.

Comment 7

[Benjamin Smedberg]

That seems like a surprising break in compatibility. dmajor, do you know about this and whether the change here is what we'd want?

Comment 8

[(Away)]

Yes that would be quite a change, if it's intentional. I don't think we should check anything in without confirmation from msft that this change is deliberate. Did we ever file that Connect bug?

Comment 9

[Mike Hommey [:glandium]]

(In reply to zhoubcfan from comment #5)
> I am familiar with the details. But the original code will give wrong
> results with vc14.
> 
> check the results on http://webcompiler.cloudapp.net/
> wprintf(L"1 %s\n","some string"); //Good
> wprintf(L"2 %s\n",L"some string"); //Not good -> print only first character
> of the string
> printf("3 %s\n","some string"); //Good
> printf("4 %s\n",L"some string"); //Not good -> print only first character of
> the string

Huh. Waw. That suggests they seriously broke compatibility.

Up to and including 2010, this is what the printf formating doc says about %s:
  When used with printf functions, specifies a single-byte–character string; when used with wprintf functions, specifies a wide-character string.
Under that specification, only cases 2 and 3 are valid.

In 2012 and 2013, this is what the printf formatting doc says about %s:
  en used with printf functions, specifies a single-byte or multi-byte character string; when used with wprintf functions, specifies a wide-character string.
Under that specification, cases 2, 3 and 4 are valid.

That cases 1 and 3 are now the valid ones seems like a serious bug.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

FYI, I didn't file a bug on connect.microsoft.com for this. It would be great if somebody from Mozilla could do so.

Comment 11

[[e-mail]]

From Microsoft Connect, I found https://connect.microsoft.com/VisualStudio/Feedback/Details/1039772

"The change in behavior brought *wprintf behavior in-line with ISO C (C99), but was a breaking change when compared to legacy behavior for Visual C++."

http://blogs.msdn.com/b/vcblog/archive/2014/06/18/crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1.aspx

"Legacy Mode: In this mode, calls to the wide string formatted I/O functions will get the legacy behavior for these three format specifiers, as they were implemented in Visual Studio 2013 and previous versions. To enable this mode, predefine the _CRT_STDIO_LEGACY_WIDE_SPECIFIERS macro when building your program."

Comment 12

[(Away)]

Thanks for that reference. I guess I'm convinced now (although surprised).

Are there any other wprintf(%s) in the codebase? What about %S?

Comment 13

[[e-mail]]

(In reply to David Major [:dmajor] (UTC+13) from comment #12)
> Thanks for that reference. I guess I'm convinced now (although surprised).
> 
> Are there any other wprintf(%s) in the codebase? What about %S?

There are. 
And "In the Visual Studio "14" CTP, we have flipped the meaning of the %c, %s, and %[] specifiers for the wide formatted I/O functions so that their behavior matches what is required by the C Standard. The meanings of the capital letter specifier equivalents (%C and %S) have also been changed, for consistency."  %c %C %[] should be checked too.

Comment 14

[[e-mail]]

dmajor, can you take this or find someone appropriate? There are code mixing narrow and wide string. I'm not confident to make the change.

Comment 15

[(Away)]

This doesn't seem urgent unless it's blocking further compilation with VC14. We can leave it assigned to "nobody" so that anyone can jump in. (I may work on it eventually, but I can't promise a time frame.)

In the meantime we might as well take the nsPluginsDirWin patch as a start.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8548575 [details] [diff] [review]
use ls instead s in format string

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

Thanks for this patch! It's checked into mozilla-inbound now. It will be in Firefox Nightly either tomorrow or the next day.

Leaving the bug open since we need to find any other cases where we're using the wrong format specifier.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Gregory Szorc [:gps]]

This bug is blocking the rollout of Visual Studio 2015, which we'd like to do soon.

Could someone please provide an update on its status?

Comment 19

[[e-mail]]

(In reply to Gregory Szorc [:gps] from comment #18)
> This bug is blocking the rollout of Visual Studio 2015, which we'd like to
> do soon.
> 
> Could someone please provide an update on its status?

You can get the wrong usages by doing a build with the related warnings enabled which mentioned in the blog(https://blogs.msdn.microsoft.com/vcblog/2015/06/22/format-specifiers-checking/).

Comment 20

[Gregory Szorc [:gps]]

Bug 1124033 is enabling all the new VS2015 warnings that are currently disabled. So marking this one as a dupe.

Comment 21

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.