Possible unintended usage of "current_function_monitor" variable
Reported by
pet...@gmail.com,
May 22 2017
|
||||
Issue description
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:53.0) Gecko/20100101 Firefox/53.0
Steps to reproduce the problem:
Hi,
While experimenting with a CodeSonar plugin we develop, we noticed a potential bug in file " chrome / browser / extensions / activity_log / activity_log.cc" line 526:
if (!current_function_monitor)
activity_monitor::SetApiEventMonitor(&LogApiEvent);
It looks that you should check the current_event_monitor variable (but you check current_function_monitor). The entire block looks to be dedicated for an *event* and not for a "function".
Thank you.
What is the expected behavior?
What went wrong?
The problem has been detected automatically at the source code level.
Did this work before? N/A
Chrome version: Master Branch Channel: n/a
OS Version: OS X 10.12
Flash Version: Shockwave Flash 25.0 r0
,
May 24 2017
(Mac triage) Looks legit to me. rdevlin.cronin@ are you the best person to look at this?
,
May 26 2017
Yep, this is a bug. Surprisingly, in practice it doesn't make a huge amount of difference, since these are set at the same time, but it should definitely be fixed in the interest of correctness.
,
May 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5554098dcecdd937d4bd96cc8017c38c9b0fecd9 commit 5554098dcecdd937d4bd96cc8017c38c9b0fecd9 Author: rdevlin.cronin <rdevlin.cronin@chromium.org> Date: Tue May 30 21:04:39 2017 [Extensions] Fix bad check in SetActivityHandlers() One of the checks in SetActivityHandlers() was checking the wrong variable. This wasn't a huge deal since these are normally all set at the same time, but it was logically incorrect. Fix it. BUG= 725102 Review-Url: https://codereview.chromium.org/2906183002 Cr-Commit-Position: refs/heads/master@{#475670} [modify] https://crrev.com/5554098dcecdd937d4bd96cc8017c38c9b0fecd9/chrome/browser/extensions/activity_log/activity_log.cc
,
May 30 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by pet...@gmail.com
, May 24 2017Some more context to better see the problem: activity_monitor::Monitor current_event_monitor = activity_monitor::GetApiEventMonitor(); DCHECK(!current_event_monitor || current_event_monitor == &LogApiEvent); if (!current_function_monitor) //Line 526 with the issue activity_monitor::SetApiEventMonitor(&LogApiEvent); As can be observed, current_event_monitor is obtain at the beginning of the block but current_function_monitor is checked in 526 (already checked in line 520). At line 526 I guess current_event_monitor should have been checked.