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

Issue 859956 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

Possible access to unintended variable in "chromium/src/components/sync/syncable/directory.cc" line 166

Reported by pet...@gmail.com, Jul 3

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce the problem:
While experimenting with a CodeSonar plugin we develop, we noticed a potential bug in file "chromium/src/components/sync/syncable/directory.cc" line 166, function  Directory::InitializeIndices.

if (!entry->ref(UNIQUE_CLIENT_TAG).empty()) {
      DCHECK(kernel_->server_tags_map.find(entry->ref(UNIQUE_SERVER_TAG)) == //HERE
             kernel_->server_tags_map.end())
          << "Unexpected duplicate use of server tag";
      kernel_->client_tags_map[entry->ref(UNIQUE_CLIENT_TAG)] = entry;
}

Shouldn't UNIQUE_CLIENT_TAG be used instead of the UNIQUE_SERVER_TAG? And similarly, shouldn't you search in the client_tags_map?

Thanks,
Petru Florin Mihancea

What is the expected behavior?
The problem has been detected automatically via static analysis.

What went wrong?
The problem has been detected automatically via static analysis.

Did this work before? N/A 

Chrome version: 67.0.3396.99  Channel: stable
OS Version: OS X 10.13.5
Flash Version:
 
Cc: maxbogue@chromium.org
Components: Services>Sync
Cc: mastiz@chromium.org
Labels: -Pri-2 Sync-Triaged sync-fixit-2018q3 OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Windows Pri-3
Status: Available (was: Unconfirmed)
Thanks for filing the bug!

I agree the DCHECK seems wrong, including the corresponding string message (as well as in line 162).
Owner: mastiz@chromium.org
Status: Started (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 6

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

commit 90dc497fafdb62268e20e73468e55d5330212db2
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Jul 06 09:08:28 2018

Fix useless DCHECK accessing unintended map

The code looks obviously wrong and the DCHECK verifies something
useless, because either a) the looked up key is empty and hence nothing
will be found; or b) the previous DCHECK a few lines above has already
verified that there's no collision.

Bug:  859956 
Change-Id: I038378bdecbf67ce5f0011c08a96d9dfbad540ac
Reviewed-on: https://chromium-review.googlesource.com/1127373
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572922}
[modify] https://crrev.com/90dc497fafdb62268e20e73468e55d5330212db2/components/sync/syncable/directory.cc

Status: Fixed (was: Started)

Sign in to add a comment