New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 643842 link

Starred by 2 users
Status: Fixed
Owner:
█ brettw@chromium.org
Last visit 26 days ago
Closed: Sep 2016
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

bugs in gn found by /analyze

Project Member Reported by brucedaw...@chromium.org, Sep 2 2016 
After hitting a format string bug in gn I ran /analyze on it (VC++ static analysis). It found the format string bug (yay!) and a few other minor issues:

tools\gn\exec_process.cc(92) : warning C6335: Leaking process information handle 'temp_process_info.hProcess'.
tools\gn\exec_process.cc(92) : warning C6335: Leaking process information handle 'temp_process_info.hThread'.
Process and thread handles are leaked. For cleanliness they should be closed, but the OS will clean them up eventually.

tools\gn\command_clean.cc(86) : warning C6303: Format string mismatch:  wide character string passed as _Param_(2) when character string is required in call to 'StringPrintf' Actual type: 'const wchar_t *'.
The bug that I found manually before! (crrev.com/2308113002)

tools\gn\command_format.cc(332) : warning C6011: Dereferencing NULL pointer 'binop'.
tools\gn\command_format.cc(875) : warning C6011: Dereferencing NULL pointer 'end'.
In both these cases the code checks for nullptr in some cases but not in others. If the existing checks are needed then more checks are needed, and vice-versa.

tools\gn\string_utils.cc(219) : warning C6330: 'const char' passed as _Param_(1) when 'unsigned char' is required in call to 'isxdigit'.
tools\gn\string_utils.cc(220) : warning C6330: 'const char' passed as _Param_(1) when 'unsigned char' is required in call to 'isxdigit'.
Passing signed char to isxdigit or its friends is not legal and can lead to undefined behavior. I don't know if it is a problem in practice.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 6 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e17ac37d6c181f2d4ec5ac51eda4717af3505596

commit e17ac37d6c181f2d4ec5ac51eda4717af3505596
Author: brucedawson <brucedawson@chromium.org>
Date: Tue Sep 06 17:43:51 2016

Fix printing of directory name on gn clean failure

The 'gn clean' code that detects whether something is a build directory
was, on Windows, printing a wchar_t* string using %s with a char*
format string. This means that only the drive letter gets printed.
Fixed by using FilePathToUTF8().

Running /analyze on gn confirmed that this is the only bug of its type
and found a few other more minor glitches, filed as part of the
referenced bug.

BUG= 643842 

Review-Url: https://codereview.chromium.org/2308113002
Cr-Commit-Position: refs/heads/master@{#416651}

[modify] https://crrev.com/e17ac37d6c181f2d4ec5ac51eda4717af3505596/tools/gn/command_clean.cc

Comment 2 by brettw@chromium.org, Sep 29 2016

Status: Started (was: Assigned)
The CreateProcess handles are cleaned up by the ScopedProcessInformation object used right after the call. The static analyzer doesn't know about this. So the first warning is a false positive.

The two null pointer dereferences are both pointers that don't need to be null checked. I'll remove the unnecessary checks that are confusing it.
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 29 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e23e7021f9a06d621088bc13856db9ba93c328c0

commit e23e7021f9a06d621088bc13856db9ba93c328c0
Author: brettw <brettw@chromium.org>
Date: Thu Sep 29 21:57:12 2016

Fix GN issues identified by MSVC static analysis.

MSVC's /analyze identified two missing null checks because it noted that a
pointer was null checked in one place but not another. After some study I
determined that null checks are not necessary for these values so I removed
them.

Although isxdigit takes an int, the input must be an unsigned char or the
behavior is undefined. I added the cast. I think this API is suboptimal.

BUG= 643842 , 427616 

Review-Url: https://codereview.chromium.org/2384493003
Cr-Commit-Position: refs/heads/master@{#421945}

[modify] https://crrev.com/e23e7021f9a06d621088bc13856db9ba93c328c0/tools/gn/command_format.cc

Comment 4 by brettw@chromium.org, Sep 29 2016

Status: Fixed (was: Started)
We decided not to address the isxdigit warnings which make the code less readable.
►

Sign in to add a comment