New issue
Advanced search Search tips

Issue 690271 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 82385



Sign in to add a comment

SecurityTest.NewOverflow fails on clang-cl

Project Member Reported by ukai@chromium.org, Feb 9 2017

Issue description

it is enabled at https://chromium.googlesource.com/chromium/src/+/82681d8ead6aa059caf6a64e485dbc6b210339be 
and start failing base_unittest on CrWinClangGoma (building with clang-cl)

https://build.chromium.org/p/chromium.fyi/builders/CrWinClangGoma/builds/22705


https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2FCrWinClangGoma%2F22705%2F%2B%2Frecipes%2Fsteps%2Fbase_unittests%2F0%2Flogs%2FSecurityTest.NewOverflow%2F0
SecurityTest.NewOverflow (run #1):
[ RUN      ] SecurityTest.NewOverflow
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
[  FAILED  ] SecurityTest.NewOverflow (0 ms)

SecurityTest.NewOverflow (run #2):
[ RUN      ] SecurityTest.NewOverflow
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
[  FAILED  ] SecurityTest.NewOverflow (0 ms)

SecurityTest.NewOverflow (run #3):
[ RUN      ] SecurityTest.NewOverflow
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
[  FAILED  ] SecurityTest.NewOverflow (0 ms)

SecurityTest.NewOverflow (run #4):
[ RUN      ] SecurityTest.NewOverflow
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
../../base/security_unittest.cc(85): error: Value of: overflow_detected
  Actual: false
Expected: true
[  FAILED  ] SecurityTest.NewOverflow (0 ms)
 

Comment 1 by siggi@chromium.org, Feb 9 2017

This seems bad, as this build is a non-component, official release build. I'd speculate that this likely opens a vulnerability in clang-built Chrome binaries (though I don't pretend to understand what the test is about).

I don't see any clang-related configuration in allocator.gni that would explain this.

Writing """\
goma_dir = "E:\\b\\c\\cipd\\goma"
is_clang = true
is_component_build = false
is_debug = false
is_official_build = true
symbol_level = 1
target_cpu = "x86"
use_goma = true
""" to E:\b\c\b\CrWinClangGoma\src\out\Release\args.gn.

Cc: r...@chromium.org
Are we seeing this on any of our bots?
Blocking: 82385

Comment 4 by siggi@chromium.org, Feb 9 2017

I'm trying to repro locally, see if I can figure what's up here.

Comment 5 by siggi@chromium.org, Feb 9 2017

Looks like the compiler is simply defeating the "HideValueFromCompiler" function. Not much to see here, though I don't know how defeat Clang for the purpose.
Nico?

See https://cs.chromium.org/chromium/src/base/security_unittest.cc?q=securitytest&l=38

Comment 7 by thakis@chromium.org, Feb 10 2017

Components: Build
Labels: clang
To answer my question from comment 2: Yes, this is broken on our bots too, see e.g. https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28shared%29%20tester/builds/9073

Comment 8 by thakis@chromium.org, Feb 10 2017

(and it's caused by https://codereview.chromium.org/2679153002 which reenabled the test on windows)

Comment 9 by thakis@chromium.org, Feb 10 2017

The usual way to do this would be

  volatile size_t kDynamicArraySize2 = kArraySize2;

instead of

  const size_t kDynamicArraySize2 = HideValueFromCompiler(kArraySize2);

Does that work?
siggi: does that help?
relanding with fix here: https://codereview.chromium.org/2694873002/
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 13 2017

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

commit 9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866
Author: thakis <thakis@chromium.org>
Date: Mon Feb 13 21:31:17 2017

Reenable SecurityTest.NewOverflow on Windows, with clang fix

Relands https://codereview.chromium.org/2679153002 with a fix for clang.

BUG= 690271 

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

[modify] https://crrev.com/9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866/base/security_unittest.cc

Owner: thakis@chromium.org
Status: Fixed (was: Untriaged)
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 14 2017

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

commit 906ce688ee9742245387dd42581020c905ab0e86
Author: gcasto <gcasto@chromium.org>
Date: Tue Feb 14 00:10:41 2017

Revert of Reenable SecurityTest.NewOverflow on Windows, with clang fix (patchset #2 id:20001 of https://codereview.chromium.org/2694873002/ )

Reason for revert:
This causes failures in the Mac ASAN bots: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29

Original issue's description:
> Reenable SecurityTest.NewOverflow on Windows, with clang fix
>
> Relands https://codereview.chromium.org/2679153002 with a fix for clang.
>
> BUG= 690271 
>
> Review-Url: https://codereview.chromium.org/2694873002
> Cr-Commit-Position: refs/heads/master@{#450098}
> Committed: https://chromium.googlesource.com/chromium/src/+/9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866

TBR=siggi@chromium.org,thakis@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 690271 

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

[modify] https://crrev.com/906ce688ee9742245387dd42581020c905ab0e86/base/security_unittest.cc

Status: Started (was: Fixed)
I added some judicious `volatile`, which made the test actually test something on the non-win platforms, which then made it fail under mac asan (the test doesn't run on linux): https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/27082

SecurityTest.NewOverflow (run #1):
[ RUN      ] SecurityTest.NewOverflow
==77609==WARNING: AddressSanitizer failed to allocate 0xffffffffffffffff bytes
==77609==AddressSanitizer's allocator is terminating the process instead of returning 0
==77609==If you don't like this behavior set allocator_may_return_null=1
==77609==AddressSanitizer CHECK failed: /b/build/slave/mac_upload_clang/build/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:221 "((0)) != (0)" (0x0, 0x0)
    #0 0x1102a412f in 0x0005e12f (in libclang_rt.asan_osx_dynamic.dylib) + 95
    #1 0x1102b885f in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (in libclang_rt.asan_osx_dynamic.dylib) + 79

Received signal 6
 [0x00010ebe0f9c]
 [0x00010ebe0cf5]
 [0x7fff8a7695aa]
 [0x7fff6e8b608a]
 [0x7fff86685b2e]
 [0x0001102bcff1]
 [0x0001102b8860]
[end of stack trace]



Relevant bit:

==77609==AddressSanitizer's allocator is terminating the process instead of returning 0
==77609==If you don't like this behavior set allocator_may_return_null=1


Somewhat related: https://github.com/google/sanitizers/issues/295


Ideally we'd want a flag that sets allocator_may_return_null only for std::nothrow news. I'm not sure if such a thing exists, so I'll reland this with the test disabled on asan, I suppose.
Project Member

Comment 16 by bugdroid1@chromium.org, Feb 14 2017

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

commit 4d7b56b40717cfe0d41f7f11de12b024a1dd6fca
Author: thakis <thakis@chromium.org>
Date: Tue Feb 14 16:21:35 2017

Reland of nable SecurityTest.NewOverflow on Windows, with clang fix (patchset #1 id:1 of https://codereview.chromium.org/2692183002/ )

Reason for revert:
Reland with test disabled on ASan.

Original issue's description:
> Revert of Reenable SecurityTest.NewOverflow on Windows, with clang fix (patchset #2 id:20001 of https://codereview.chromium.org/2694873002/ )
>
> Reason for revert:
> This causes failures in the Mac ASAN bots: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29
>
> Original issue's description:
> > Reenable SecurityTest.NewOverflow on Windows, with clang fix
> >
> > Relands https://codereview.chromium.org/2679153002 with a fix for clang.
> >
> > BUG= 690271 
> >
> > Review-Url: https://codereview.chromium.org/2694873002
> > Cr-Commit-Position: refs/heads/master@{#450098}
> > Committed: https://chromium.googlesource.com/chromium/src/+/9b14da5b9f4f5c2b78bc1b64fa590e126f0f2866
>
> TBR=siggi@chromium.org,thakis@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 690271 
>
> Review-Url: https://codereview.chromium.org/2692183002
> Cr-Commit-Position: refs/heads/master@{#450166}
> Committed: https://chromium.googlesource.com/chromium/src/+/906ce688ee9742245387dd42581020c905ab0e86

TBR=siggi@chromium.org,gcasto@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 690271 

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

[modify] https://crrev.com/4d7b56b40717cfe0d41f7f11de12b024a1dd6fca/base/security_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 18 by bugdroid1@chromium.org, Feb 21 2017

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

commit a71e1c4d8c9bb54716c7ee7f82ce6b99730bb1b7
Author: thakis <thakis@chromium.org>
Date: Tue Feb 21 17:20:41 2017

Enable SecurityTest.NewOverflow on Linux now that valgrind is gone

BUG= 582398 , 690271 

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

[modify] https://crrev.com/a71e1c4d8c9bb54716c7ee7f82ce6b99730bb1b7/base/security_unittest.cc

Sign in to add a comment