Detecting and fixing stringprintf.h format bugs

Detecting and fixing stringprintf.h format bugs

The print functions in stringprintf.h were not annotated for /analyze so
13 Windows specific format-string bugs accumulated. This annotates the
functions so that the /analyze builder will find the problems and fixes
the bugs.

R=thestig@chromium.org,wfh@chromium.org,jam@chromium.org
Skipping wstring presubmit checks - no new wstring usage is added
NOPRESUBMIT=true
BUG=427616
Committed: https://crrev.com/5604a11d546e02859db5dc75af72ce27bc14c158
Cr-Commit-Position: refs/heads/master@{#352659}

[brucedawson, 2015-09-28 17:27:56]

I ran /analyze with some extra annotations over the weekend and found thirteen
bugs. They were all wide/narrow string mismatches or 64-bit integer bugs. I
don't think they are serious but they should be fixed.

I have not yet done a local build to verify that these correct the warnings -
that will happen tonight.

[jam, 2015-09-28 20:25:04]

On 2015/09/28 17:27:56, brucedawson wrote:
> I ran /analyze with some extra annotations over the weekend and found thirteen
> bugs. They were all wide/narrow string mismatches or 64-bit integer bugs. I
> don't think they are serious but they should be fixed.
> 
> I have not yet done a local build to verify that these correct the warnings -
> that will happen tonight.

someone from base/owners should review this.

[brucedawson, 2015-09-28 20:38:56]

The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run

[brucedawson, 2015-09-28 20:40:50]

brucedawson@chromium.org changed reviewers:
+ thestig@chromium.org
- jam@chromium.org

[commit-bot: I haz the power, 2015-09-28 20:42:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372153002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372153002/1

[brucedawson, 2015-09-28 20:51:03]

brucedawson@chromium.org changed reviewers:
+ jam@chromium.org

[brucedawson, 2015-09-28 20:52:20]

Added thestig@ for review of base changes. I also added four fixes to
one line of code that I'd previously made in the wrong repo.

[Will Harris, 2015-09-29 00:29:17]

only reviewed content/common/sandbox_win.cc

https://codereview.chromium.org/1372153002/diff/20001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1372153002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:227: static uint64_t s_session_id = 0;
This should be DWORD, since GetTokenInformation with TokenSessionId always
returns a DWORD

https://codereview.chromium.org/1372153002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:241: return
base::StringPrintf(L"\\Sessions\\%llu%ls", s_session_id, object);
%lu seems sensible here?

[Lei Zhang, 2015-09-29 03:16:17]

https://codereview.chromium.org/1372153002/diff/20001/base/strings/stringprin...
File base/strings/stringprintf.h (right):

https://codereview.chromium.org/1372153002/diff/20001/base/strings/stringprin...
base/strings/stringprintf.h:10: #ifdef _MSC_VER
Check against COMPILER_MSVC instead? This is something clang+windows does not
have?

https://codereview.chromium.org/1372153002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/1372153002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc:52:
#ifdef OS_WIN
#if defined(OS_WIN), ditto elsewhere, but maybe just do:

#if defined(OS_WIN)
#define FP_MACRO ls
#else
#define FP_MACRO s
#endif
*error = base::StringPrintf("File does not exist: %" FP_MACRO,
file.value().c_str());

[brucedawson, 2015-09-29 21:39:40]

Thanks for the feedback. I'll put up another CL momentarily.

https://codereview.chromium.org/1372153002/diff/20001/base/strings/stringprin...
File base/strings/stringprintf.h (right):

https://codereview.chromium.org/1372153002/diff/20001/base/strings/stringprin...
base/strings/stringprintf.h:10: #ifdef _MSC_VER
On 2015/09/29 03:16:17, Lei Zhang wrote:
> Check against COMPILER_MSVC instead? This is something clang+windows does not
> have?

I'm not sure what the best check is. The need for the check is because nacl
builds cannot include sal.h. It doesn't necessarily matter whether the compiler
*understands* sal.h, it just has to be able to find it.

A nacl check seems ideal, but _MSC_VER or COMPILER_MSVC should work.

https://codereview.chromium.org/1372153002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/1372153002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc:52:
#ifdef OS_WIN
On 2015/09/29 03:16:17, Lei Zhang wrote:
> #if defined(OS_WIN), ditto elsewhere, but maybe just do:
> 
> #if defined(OS_WIN)
> #define FP_MACRO ls
> #else
> #define FP_MACRO s
> #endif
> *error = base::StringPrintf("File does not exist: %" FP_MACRO,
> file.value().c_str());

Yes. That is much better. It is cleaner and by defining FP_MACRO near
FilePath::StringType it localizes that knowledge.

https://codereview.chromium.org/1372153002/diff/20001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1372153002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:227: static uint64_t s_session_id = 0;
On 2015/09/29 00:29:17, Will Harris wrote:
> This should be DWORD, since GetTokenInformation with TokenSessionId always
> returns a DWORD

Done.

https://codereview.chromium.org/1372153002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:241: return
base::StringPrintf(L"\\Sessions\\%llu%ls", s_session_id, object);
On 2015/09/29 00:29:17, Will Harris wrote:
> %lu seems sensible here?

If s_session_id is a DWORD then %lu does seem best.

[Lei Zhang, 2015-09-29 21:42:37]

https://codereview.chromium.org/1372153002/diff/20001/base/strings/stringprin...
File base/strings/stringprintf.h (right):

https://codereview.chromium.org/1372153002/diff/20001/base/strings/stringprin...
base/strings/stringprintf.h:10: #ifdef _MSC_VER
On 2015/09/29 21:39:39, brucedawson wrote:
> On 2015/09/29 03:16:17, Lei Zhang wrote:
> > Check against COMPILER_MSVC instead? This is something clang+windows does
not
> > have?
> 
> I'm not sure what the best check is. The need for the check is because nacl
> builds cannot include sal.h. It doesn't necessarily matter whether the
compiler
> *understands* sal.h, it just has to be able to find it.
> 
> A nacl check seems ideal, but _MSC_VER or COMPILER_MSVC should work.

There's no hard rule, but in non-third-party code, COMPILER_MSVC usage
outnumbers _MSC_VER 2:1.

https://codereview.chromium.org/1372153002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/1372153002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc:52:
#ifdef OS_WIN
On 2015/09/29 21:39:39, brucedawson wrote:
> On 2015/09/29 03:16:17, Lei Zhang wrote:
> > #if defined(OS_WIN), ditto elsewhere, but maybe just do:
> > 
> > #if defined(OS_WIN)
> > #define FP_MACRO ls
> > #else
> > #define FP_MACRO s
> > #endif
> > *error = base::StringPrintf("File does not exist: %" FP_MACRO,
> > file.value().c_str());
> 
> Yes. That is much better. It is cleaner and by defining FP_MACRO near
> FilePath::StringType it localizes that knowledge.

SGTM

[brucedawson, 2015-09-29 21:50:03]

> > Yes. That is much better. It is cleaner and by defining FP_MACRO near
> > FilePath::StringType it localizes that knowledge.
> 
> SGTM

I went with PRIsFP - it seems more compatible with the PRIdNS etc. naming
convention.

[brucedawson, 2015-09-30 17:09:00]

Change should be final now.

I addressed all of the code-review feedback, fixed one bug that I'd missed in
model_type_worker.cc, pulled the latest code, and confirmed via an overnight
/analyze run that all of the stringprintf bugs are fixed.

If you know of other frequently used printf-type functions that could use
annotation then let me know for future investigations.

PTAL

[Will Harris, 2015-09-30 17:18:34]

content/common/sandbox_win.cc lgtm

[Lei Zhang, 2015-09-30 18:35:02]

lgtm, with more documentation:

https://codereview.chromium.org/1372153002/diff/80001/base/files/file_path.h
File base/files/file_path.h (right):

https://codereview.chromium.org/1372153002/diff/80001/base/files/file_path.h#...
base/files/file_path.h:141: #define PRIsFP "s"
You should move these into their own section with documentation for how to use
them, including an example. Not everyone is going to figure out what this is.

"Isn't a PRIuS some kind of a car?"

[Lei Zhang, 2015-09-30 18:35:12]

base/ and chrome/ lgtm, that is.

[brucedawson, 2015-09-30 20:24:05]

On 2015/09/30 18:35:12, Lei Zhang wrote:
> base/ and chrome/ lgtm, that is.

Lei - I added a comment to file_path.h but I left the #defines where they are
because it seems important to have them right beside the typedef which they must
match. Thoughts?

[Lei Zhang, 2015-09-30 22:12:43]

On 2015/09/30 20:24:05, brucedawson wrote:
> On 2015/09/30 18:35:12, Lei Zhang wrote:
> > base/ and chrome/ lgtm, that is.
> 
> Lei - I added a comment to file_path.h but I left the #defines where they are
> because it seems important to have them right beside the typedef which they
must
> match. Thoughts?

How about we move them up to line 126, rather than cramming it in with the
FilePath decls? This will likely not change again any time soon, and we can
trust base/OWNERS to make sure this matches the FilePath::StringType typdefs
below.

// To print path names portably use PRIsFP...
#if defined(OS_POSIX)
#define PRIsFP "s"
#elif defined(OS_WIN)
#define PRIsFP "ls"
#endif

[brucedawson, 2015-09-30 23:02:54]

Sure, that makes sense. Done.

And, if somebody did change PRIsFP or StringType then the /analyze or gcc/clang
warnings would alert us pretty quickly.

Lei, PTAL

[Lei Zhang, 2015-09-30 23:07:29]

On 2015/09/30 23:02:54, brucedawson wrote:
> Sure, that makes sense. Done.
> 
> And, if somebody did change PRIsFP or StringType then the /analyze or
gcc/clang
> warnings would alert us pretty quickly.
> 
> Lei, PTAL

Sorry to be nit-picky, but I would prefer for the FILE_PATH_USES_* #defines to
remain in a separate #if block, rather than mixing the two.

[brucedawson, 2015-09-30 23:33:33]

On 2015/09/30 23:07:29, Lei Zhang wrote:
> On 2015/09/30 23:02:54, brucedawson wrote:
> > Sure, that makes sense. Done.
> > 
> > And, if somebody did change PRIsFP or StringType then the /analyze or
> gcc/clang
> > warnings would alert us pretty quickly.
> > 
> > Lei, PTAL
> 
> Sorry to be nit-picky, but I would prefer for the FILE_PATH_USES_* #defines to
> remain in a separate #if block, rather than mixing the two.

No worries. I can see the pros of both methods. Changed. PTAL.

[Lei Zhang, 2015-09-30 23:39:46]

On 2015/09/30 23:33:33, brucedawson wrote:
> On 2015/09/30 23:07:29, Lei Zhang wrote:
> > On 2015/09/30 23:02:54, brucedawson wrote:
> > > Sure, that makes sense. Done.
> > > 
> > > And, if somebody did change PRIsFP or StringType then the /analyze or
> > gcc/clang
> > > warnings would alert us pretty quickly.
> > > 
> > > Lei, PTAL
> > 
> > Sorry to be nit-picky, but I would prefer for the FILE_PATH_USES_* #defines
to
> > remain in a separate #if block, rather than mixing the two.
> 
> No worries. I can see the pros of both methods. Changed. PTAL.

++lgtm. This is easier for readers to comprehend.

[jabdelmalek, 2015-09-30 23:40:46]

lgtm

[brucedawson, 2015-10-01 01:02:41]

The CQ bit was checked by brucedawson@chromium.org

[brucedawson, 2015-10-01 01:02:41]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset:
https://chromiumcodereview-hr.appspot.com/1372153002/#ps140001 (title: "Separate
#if defined blocks.")

[commit-bot: I haz the power, 2015-10-01 01:04:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372153002/140001

[commit-bot: I haz the power, 2015-10-01 01:17:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 01:17:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brucedawson, 2015-10-02 17:32:30]

On 2015/10/01 01:17:32, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

jam@ can I get approval from your jam@ account? Your jabdelmalek@ account
doesn't own anything. Thanks.

[jam, 2015-10-05 23:23:57]

On 2015/10/02 17:32:30, brucedawson wrote:
> On 2015/10/01 01:17:32, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> jam@ can I get approval from your jam@ account? Your jabdelmalek@ account
> doesn't own anything. Thanks.

lgtm, sorry

[brucedawson, 2015-10-05 23:27:33]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-10-05 23:28:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372153002/140001

[commit-bot: I haz the power, 2015-10-05 23:43:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-05 23:43:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brucedawson, 2015-10-06 18:25:19]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-10-06 18:26:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372153002/140001

[commit-bot: I haz the power, 2015-10-06 19:22:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-06 19:23:16]

Patchset 8 (id:??) landed as
https://crrev.com/5604a11d546e02859db5dc75af72ce27bc14c158
Cr-Commit-Position: refs/heads/master@{#352659}