New issue
Advanced search Search tips

Issue 861726 link

Starred by 2 users
Status: Fixed
Owner: ----
Closed: Jul 29
Cc:
kenrb@chromium.org
thakis@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

MemorySanitizer: use-of-uninitialized-value

Reported by dima00...@gmail.com, Jul 9 
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 YaBrowser/18.4.1.868 Yowser/2.5 Safari/537.36

Steps to reproduce the problem:
1. build chromium with msan
2. run base_unittests.ProcessTest.WaitForExitWithTimeout

What is the expected behavior?
no errors

What went wrong?
==414845==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x52d50e in CmpHelperEQ<int, int> /src/third_party/googletest
    #1 0x52d50e in testing::AssertionResult testing::internal::EqHelper<false>::Compare<int, int>(char const*, char const*, int const&, int const&) /src/third_party/googletest
    #2 0x1b9a25a in base::ProcessTest_WaitForExitWithTimeout_Test::TestBody() /src/base/process/process_unittest.cc:220:3
    #3 0x3aede8d in testing::Test::Run() /src/third_party/googletest
    #4 0x3af15eb in testing::TestInfo::Run() /src/third_party/googletest
    #5 0x3af3009 in testing::TestCase::Run() /src/third_party/googletest
    #6 0x3b29844 in testing::internal::UnitTestImpl::RunAllTests() /src/third_party/googletest
    #7 0x3b28342 in testing::UnitTest::Run() /src/third_party/googletest
    #8 0x4035474 in RUN_ALL_TESTS /src/third_party/googletest
    #9 0x4035474 in base::TestSuite::Run() /src/base/test/test_suite.cc:275:0
    #10 0x40655e3 in Run /src/base/callback.h:96:12
    #11 0x40655e3 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) /src/base/test/launcher/unit_test_launcher.cc:225:0
    #12 0x4064c5e in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) /src/base/test/launcher/unit_test_launcher.cc:582:10
    #13 0x4001b91 in main /src/base/test/run_all_base_unittests.cc:12:10
    #14 0x7fd7a91b8f44 in __libc_start_main /src/??:0:0
    #15 0x4b9029 in _start /src/??:0:0

  Uninitialized value was stored to memory at
    #0 0x3fa4915 in base::Process::WaitForExitWithTimeout(base::TimeDelta, int*) const /src/base/process/process_posix.cc:350:18
    #1 0x1b99a69 in base::ProcessTest_WaitForExitWithTimeout_Test::TestBody() /src/base/process/process_unittest.cc:219:3
    #2 0x3aede8d in testing::Test::Run() /src/third_party/googletest
    #3 0x3af15eb in testing::TestInfo::Run() /src/third_party/googletest
    #4 0x3af3009 in testing::TestCase::Run() /src/third_party/googletest
    #5 0x3b29844 in testing::internal::UnitTestImpl::RunAllTests() /src/third_party/googletest
    #6 0x3b28342 in testing::UnitTest::Run() /src/third_party/googletest
    #7 0x4035474 in RUN_ALL_TESTS /src/third_party/googletest
    #8 0x4035474 in base::TestSuite::Run() /src/base/test/test_suite.cc:275:0
    #9 0x40655e3 in Run /src/base/callback.h:96:12
    #10 0x40655e3 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) /src/base/test/launcher/unit_test_launcher.cc:225:0
    #11 0x4064c5e in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) /src/base/test/launcher/unit_test_launcher.cc:582:10
    #12 0x4001b91 in main /src/base/test/run_all_base_unittests.cc:12:10
    #13 0x7fd7a91b8f44 in __libc_start_main /src/??:0:0

  Uninitialized value was created
    <empty stack>

SUMMARY: MemorySanitizer: use-of-uninitialized-value (/place/sandbox-data/tasks/7/2/264344627/btr_isolate_6FoGYB/src/out/Default/base_unittests+0x52d50e)

Did this work before? N/A 

Chrome version: 65.0.3325.181  Channel: n/a
OS Version: 3.18.43-40
Flash Version:

Comment 1 by kenrb@chromium.org, Jul 9

Labels: Needs-Feedback
I can't repro this on trunk.

Does this only work on Chrome 65 for you? That is an old version.

Comment 2 by kenrb@chromium.org, Jul 9 

 Issue 861727  has been merged into this issue.

Comment 3 by dima00...@gmail.com, Jul 10 

Yes, thx. I've filed two equal issues from different accounts.
I'm from Yandex Browser team and this things affect us. I've created patch for this - https://chromium-review.googlesource.com/c/chromium/src/+/1126927.

Comment 4 by dima00...@gmail.com, Jul 10 

Also, Can you add Nico Weber (thakis@chromium.org) to cc? He is reviewer of my patch.
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 10

Cc: kenrb@chromium.org
Labels: -Needs-Feedback
Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by kenrb@chromium.org, Jul 10

Cc: thakis@chromium.org

Comment 7 by kenrb@chromium.org, Jul 10 

This doesn't immediately look like a security bug, unless there is a way for an attacker to read the uninitialized value.

Also I am curious why this wouldn't have come up already from our bots. We must run unit tests under MSAN.

Comment 8 by thakis@chromium.org, Jul 10 

It's a bit weird. From what I see, this happens if WaitForExitWithTimeoutImpl returns true from the early return which doesn't write exit_code, but that happens only if the child has already exited and then waitpi() times out (?).

Or optimization settings are different than on our bots, and this is some speculative read.
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 10 

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

commit ff22a4d365c366a01329d4a4989aba3140408151
Author: Dmitry Bezheckov <dbezheckov@yandex-team.ru>
Date: Tue Jul 10 19:42:44 2018

initialize local_exit_code with zero

R=thakis@chromium.org
Test= base_unittests.ProcessTest.WaitForExitWithTimeout. To reproduce use memory sanitizer (msan)

Bug:  861726 

Change-Id: If82e2ae32ba71572fa0ec85595b4a00f9a046a1b
Reviewed-on: https://chromium-review.googlesource.com/1126927
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573861}
[modify] https://crrev.com/ff22a4d365c366a01329d4a4989aba3140408151/base/process/process_posix.cc

Comment 10 by est...@chromium.org, Jul 10 

Is this a test-only problem, or could it be a problem in production code as well?

Comment 11 by est...@chromium.org, Jul 12 

Friendly ping to thakis -- could you clarify if this is a test-only bug or not? Thanks!

Comment 12 by thakis@chromium.org, Jul 13 

I'm not sure, but my guess is that this affects production code too, but my hunch is that this is probably an artifact of the interaction of reordering by the compiler and how msan's instrumentation works. The compiler is allowed to reorder things and do speculative loads as long as they don't change behavior, and e.g. read uninitialized memory if that read is guarded by a flag if it makes sure to only use the data it has read if the flag was set -- but msan will catch the uninit read and not know about flag checks that might happen later in the generated code (but earlier in the cc file). So my guess is "not test only, but not security relevant".

Comment 13 by est...@chromium.org, Jul 13

Labels: Security_Impact-None
Ok, thanks, I'm going to triage as Security_Impact-None but leave it view-restricted just in case there turn out to be security implications.

Comment 14 by meacer@google.com, Jul 27 

Has this been fixed?

Comment 15 by mbarbe...@chromium.org, Jul 27

Labels: -Type-Bug-Security -Restrict-View-SecurityTeam -Security_Impact-None Type-Bug
Removing from the security queue based on c#12.

Comment 16 by thakis@chromium.org, Jul 29

Status: Fixed (was: Unconfirmed)
Yes, fixed by dbezheckov@yandex-team.ru
►

Sign in to add a comment