Bugs found in Chrome by PVS-Studio
Kostya Serebryany
Hans Wennborg
AFAICT, these are the issues mentioned in the blog post:
>
> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216
> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010
> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) {
> ...
> info->is_directory =
> file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0;
> ...
> }
>
> Должно быть:
> (file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0;
Clang catches this with "warning: & has lower precedence than !=; !=
will be evaluated first" (-Wparentheses)
>
> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227
> void Time::Explode(bool is_local, Exploded* exploded) const {
> ...
> ZeroMemory(exploded, sizeof(exploded));
> ...
> }
>
> Очистили только часть буфера.
Clang doesn't catch this.
>
> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404
> bool AutoFillProfileHasName(const AutoFillProfile& profile) {
> return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() ||
> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() ||
> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty();
> }
>
> Два раза NAME_MIDDLE используется.
Clang doesn't catch this.
>
> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23
> IdleState CalculateIdleState(unsigned int idle_threshold) {
> ...
> DWORD current_idle_time = 0;
> ...
> // Will go -ve if we have been idle for a long time (2gb seconds).
> if (current_idle_time < 0)
> current_idle_time = INT_MAX;
> ...
> }
>
> Должно быть:
> if (current_idle_time > INT_MAX)
Clang warns about this: "comparison of unsigned expression < 0 is
always false" (-Wtautological-compare)
>
> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293
> bool NpProxyService::GetProxyValueJSONString(std::string* output) {
> DCHECK(output);
> output->empty();
> ...
> }
>
> Спутали функцию clear() и empty().
I can't find the np_proxy_service.cc file, but I don't think Clang
would warn here.
>
> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400
> static const int kClientEdgeThickness;
> int height() const;
> bool ShouldShowClientEdge() const;
>
> void CustomFrameView::PaintMaximizedFrameBorder(gfx::Canvas* canvas) {
> ...
> int edge_height = titlebar_bottom->height() -
> ShouldShowClientEdge() ? kClientEdgeThickness : 0;
> ...
> }
>
> Забыты скобки:
> int edge_height = titlebar_bottom->height() -
> (ShouldShowClientEdge() ? kClientEdgeThickness : 0);
No warning here.
We could probably add warnings for some of these without too much
trouble. I'll look into it.
Thanks,
Hans
Evan Martin
>> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216
>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010
>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) {
>> ...
>> info->is_directory =
>> file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0;
>> ...
>> }
http://code.google.com/p/chromium/issues/detail?id=83234
>> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227
>> void Time::Explode(bool is_local, Exploded* exploded) const {
>> ...
>> ZeroMemory(exploded, sizeof(exploded));
>> ...
>> }
http://code.google.com/p/chromium/issues/detail?id=83236
>> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404
>> bool AutoFillProfileHasName(const AutoFillProfile& profile) {
>> return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() ||
>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() ||
>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty();
>> }
>>
>> Два раза NAME_MIDDLE используется.
Can't find this code in our tree.
>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23
>> IdleState CalculateIdleState(unsigned int idle_threshold) {
>> ...
>> DWORD current_idle_time = 0;
>> ...
>> // Will go -ve if we have been idle for a long time (2gb seconds).
>> if (current_idle_time < 0)
>> current_idle_time = INT_MAX;
>> ...
>> }
>>
>> Должно быть:
>> if (current_idle_time > INT_MAX)
>
> Clang warns about this: "comparison of unsigned expression < 0 is
> always false" (-Wtautological-compare)
http://code.google.com/p/chromium/issues/detail?id=83237
>> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293
>> bool NpProxyService::GetProxyValueJSONString(std::string* output) {
>> DCHECK(output);
>> output->empty();
>> ...
>> }
>>
>> Спутали функцию clear() и empty().
>
> I can't find the np_proxy_service.cc file, but I don't think Clang
> would warn here.
Can't find it either.
Nico Weber
clang's static analyzer on the other hand does I believe. The static
analyzer started supporting C++ only very recently though, and it
produces many many false positives at the moment, so that's not all
that helpful.
>
>>
>> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404
>> bool AutoFillProfileHasName(const AutoFillProfile& profile) {
>> return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() ||
>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() ||
>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty();
>> }
>>
>> Два раза NAME_MIDDLE используется.
>
> Clang doesn't catch this.
Hm, maybe it could? I filed http://llvm.org/bugs/show_bug.cgi?id=9952
>
>>
>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23
>> IdleState CalculateIdleState(unsigned int idle_threshold) {
>> ...
>> DWORD current_idle_time = 0;
>> ...
>> // Will go -ve if we have been idle for a long time (2gb seconds).
>> if (current_idle_time < 0)
>> current_idle_time = INT_MAX;
>> ...
>> }
>>
>> Должно быть:
>> if (current_idle_time > INT_MAX)
>
> Clang warns about this: "comparison of unsigned expression < 0 is
> always false" (-Wtautological-compare)
>
>>
>> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293
>> bool NpProxyService::GetProxyValueJSONString(std::string* output) {
>> DCHECK(output);
>> output->empty();
>> ...
>> }
>>
>> Спутали функцию clear() и empty().
>
> I can't find the np_proxy_service.cc file, but I don't think Clang
> would warn here.
I think clang _does_ support WARN_UNUSED_RESULT (see
base/compiler_specific.h for the definition and base/file_path.h for
example uses), just like gcc does. So both gcc and clang would've
caught this.
>
>>
>> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400
>> static const int kClientEdgeThickness;
>> int height() const;
>> bool ShouldShowClientEdge() const;
>>
>> void CustomFrameView::PaintMaximizedFrameBorder(gfx::Canvas* canvas) {
>> ...
>> int edge_height = titlebar_bottom->height() -
>> ShouldShowClientEdge() ? kClientEdgeThickness : 0;
>> ...
>> }
>>
>> Забыты скобки:
>> int edge_height = titlebar_bottom->height() -
>> (ShouldShowClientEdge() ? kClientEdgeThickness : 0);
>
> No warning here.
>
> We could probably add warnings for some of these without too much
> trouble. I'll look into it.
Can you file a llvm pr for this one then?
>
> Thanks,
> Hans
>
Hans Wennborg
http://llvm.org/bugs/show_bug.cgi?id=9969
>
>>
>> Thanks,
>> Hans
>>
>
Nico Weber
> On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <ha...@chromium.org> wrote:
>>> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216
>>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010
>>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) {
>>> ...
>>> info->is_directory =
>>> file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0;
>>> ...
>>> }
>
> http://code.google.com/p/chromium/issues/detail?id=83234
>
>>> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227
>>> void Time::Explode(bool is_local, Exploded* exploded) const {
>>> ...
>>> ZeroMemory(exploded, sizeof(exploded));
>>> ...
>>> }
>
> http://code.google.com/p/chromium/issues/detail?id=83236
Also http://llvm.org/bugs/show_bug.cgi?id=9977
Kostya Serebryany
Michael Chastain
Example:
mec@karp:~/exp-and$ cat z1.cc
int Alpha(int a, int b) {
О©╫ return a & b != 0;
}
mec@karp:~/exp-and$ /usr/crosstool/v11/gcc-4.2.2-glibc-2.3.6-grte/x86_64-unknown-linux-gnu/bin/x86_64-unknown-linux-gnu-g++ --version
x86_64-unknown-linux-gnu-g++ (GCC) 4.2.2
Copyright (C) 2007 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.О©╫ There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
mec@karp:~/exp-and$ /usr/crosstool/v11/gcc-4.2.2-glibc-2.3.6-grte/x86_64-unknown-linux-gnu/bin/x86_64-unknown-linux-gnu-g++ -Wall -Werror -c z1.cc
cc1plus: warnings being treated as errors
z1.cc: In function 'int Alpha(int, int)':
z1.cc:2: warning: suggest parentheses around comparison in operand of &
On 10/20/11 01:06, Kostya Serebryany wrote:
New findings from PVS folks (thanks Paul for keeping the watch).О©╫Should be interesting to both Chrome and Clang crowds.О©╫
From the first glance, everything could be done in clang.О©╫
--kcc
On Fri, May 20, 2011 at 4:46 PM, Nico Weber <tha...@chromium.org> wrote:
On Thu, May 19, 2011 at 9:29 AM, Evan Martin <ev...@chromium.org> wrote:
> On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <ha...@chromium.org> wrote:
>>> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216
>>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010
>>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) {
>>> О©╫ ...
>>> О©╫ info->is_directory =
>>> О©╫ О©╫ file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0;
>>> О©╫ ...
>>> }
>
> http://code.google.com/p/chromium/issues/detail?id=83234
>
>>> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227
>>> void Time::Explode(bool is_local, Exploded* exploded) const {
>>> О©╫ ...Also http://llvm.org/bugs/show_bug.cgi?id=9977
>>> О©╫ ZeroMemory(exploded, sizeof(exploded));
>>> О©╫ ...
>>> }
>
> http://code.google.com/p/chromium/issues/detail?id=83236
>
>>> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404
>>> bool AutoFillProfileHasName(const AutoFillProfile& profile) {
>>> О©╫ return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() ||
>>> О©╫ О©╫ О©╫ О©╫ О©╫!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() ||
>>> О©╫ О©╫ О©╫ О©╫ О©╫!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty();
>>> }
>>>
>>> О©╫О©╫О©╫ О©╫О©╫О©╫О©╫ NAME_MIDDLE О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫.
>
> Can't find this code in our tree.
>
>>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23
>>> IdleState CalculateIdleState(unsigned int idle_threshold) {
>>> О©╫ ...
>>> О©╫ DWORD current_idle_time = 0;
>>> О©╫ ...
>>> О©╫ // Will go -ve if we have been idle for a long time (2gb seconds).
>>> О©╫ if (current_idle_time < 0)
>>> О©╫ О©╫ current_idle_time = INT_MAX;
>>> О©╫ ...
>>> }
>>>
>>> О©╫О©╫О©╫О©╫О©╫О©╫ О©╫О©╫О©╫О©╫:
>>> if (current_idle_time > INT_MAX)
>>
>> Clang warns about this: "comparison of unsigned expression < 0 is
>> always false" (-Wtautological-compare)
>
> http://code.google.com/p/chromium/issues/detail?id=83237
>
>>> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293
>>> bool NpProxyService::GetProxyValueJSONString(std::string* output) {
>>> О©╫ DCHECK(output);
>>> О©╫ output->empty();
>>> О©╫ ...
>>> }
>>>
>>> О©╫О©╫О©╫О©╫О©╫О©╫О©╫ О©╫О©╫О©╫О©╫О©╫О©╫О©╫ clear() О©╫ empty().
>>
>> I can't find the np_proxy_service.cc file, but I don't think Clang
>> would warn here.
>
> Can't find it either.
>
>>> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400
>>> static const int kClientEdgeThickness;
>>> int height() const;
>>> bool ShouldShowClientEdge() const;
>>>
>>> void CustomFrameView::PaintMaximizedFrameBorder(gfx::Canvas* canvas) {
>>> О©╫ ...
>>> О©╫ int edge_height = titlebar_bottom->height() -
>>> О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ ShouldShowClientEdge() ? kClientEdgeThickness : 0;
>>> О©╫ ...
>>> }
>>>
>>> О©╫О©╫О©╫О©╫О©╫О©╫ О©╫О©╫О©╫О©╫О©╫О©╫:
>>> int edge_height = titlebar_bottom->height() -
>>> О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ О©╫ (ShouldShowClientEdge() ? kClientEdgeThickness : 0);
Chandler Carruth
New findings from PVS folks (thanks Paul for keeping the watch).Should be interesting to both Chrome and Clang crowds.
From the first glance, everything could be done in clang.
Paul Pluzhnikov
> Should be interesting to both Chrome and Clang crowds.
FWIW, Clang already finds the 'memset(foo, 0, sizeof(foo));' ...
http://git.savannah.gnu.org/gitweb/?p=libunwind.git;a=commit;h=f89fb17695e8137a5f4e23570bf9f53374186c96
--
Paul Pluzhnikov
Chandler Carruth
I am likely missing some context, but gcc -Wall has detected the first snippet below for several years now. Are the Chrome people not building with gcc, or do they not enable -Wall, or do they ignore warnings, or what?
Nico Weber
<ppluz...@google.com> wrote:
> On Wed, Oct 19, 2011 at 10:06 PM, Kostya Serebryany <k...@google.com> wrote:
>> New findings from PVS folks (thanks Paul for keeping the watch).
>> Should be interesting to both Chrome and Clang crowds.
>
> FWIW, Clang already finds the 'memset(foo, 0, sizeof(foo));' ...
> http://git.savannah.gnu.org/gitweb/?p=libunwind.git;a=commit;h=f89fb17695e8137a5f4e23570bf9f53374186c96
Yes, we added that. I added support for the same check in strncmp() in
response to this current post. hwennborg added the ?: warning to clang
a while ago.
We knew about several of the issues they published in this post
already because compilers warned about them, but since it was in dead
third-party code we haven't fixed it yet. (See http://crbug.com/100213
for the list).
Ryan Sleevi
http://code.google.com/p/chromium/issues/detail?id=100213
2011/10/20 Michael Chastain <m...@google.com>:
> I am likely missing some context, but gcc -Wall has detected the first
> with gcc, or do they not enable -Wall, or do they ignore warnings, or what?
>
PVS Studio runs on Windows and targets the Windows builds. This is
unfortunately one area where we lack good coverage. I understand that
Google does/did build with Coverity, which offered Windows coverage,
but Clang can not (yet) compile the Windows files and GCC on Windows
for Chromium = (heh || meh).
As far as the Coverity builds, I'm not aware of anything going on for
the non-Google Chromium development community, nor are there
buildbots, so it's an area that can and does quickly/easily fall
behind on - unlike the Mac/Linux side, which have clang buildbots
building continuously and closing the tree on errors.
Additionally, the max warnings are only turned up when chromium_code=1
in the gyp files that control the builds. By default, third-party libs
don't have this turned on - because unfortunately some of them play
pretty fast and loose. From what I've seen from the PVS Studio people,
most of their bugs tend to be reported against the third-party-deps of
Chromium. Because they lump both Chromium code and third-party code
together in their report, it's unsurprising to find that there may be
missed coverage.
Nico Weber
This is http://code.google.com/p/chromium/issues/detail?id=83234 ,
filed 5 months ago and knows longer than that. Not fixed because it's
dead code and the Real Fix is "figure out why this code is there at
all and then delete it".