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

Issue 806013 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 724628



Sign in to add a comment

libbrillo has new ASAN error (due to libc++ migration?)

Project Member Reported by cmt...@chromium.org, Jan 25 2018

Issue description

The amd64-generic-asan builder is failing in the UnitTest stage with a new ASAN error in libbrillo

See https://logs.chromium.org/v/?s=chromiumos%2Fbb%2Fchromiumos%2Famd64-generic-asan%2F23050%2F%2B%2Frecipes%2Fsteps%2FUnitTest%2F0%2Fstdout
for details.

Not sure about the right label/component for this bug.
 

Comment 1 by vapier@chromium.org, Jan 25 2018

Blocking: 724628
Cc: manojgupta@chromium.org ejc@google.com
Summary: libbrillo has new ASAN error (due to libc++ migration?) (was: libbrillo has new ASAN error)
build 23040 is the first one to show this (see attached logs):
https://uberchromegw.corp.google.com/i/chromiumos/builders/amd64-generic-asan/builds/23040

build 23031 was the last passing run:
https://uberchromegw.corp.google.com/i/chromiumos/builders/amd64-generic-asan/builds/23031

all the versions in between failed to build i think due to the libc++ migration.  so i suspect there's a difference in libc++ that's causing this to show up (either due to a bug in libbrillo or in libc++).
librillo.log
59.7 KB View Download
Owner: manojgupta@chromium.org
Taking a look.
Looks like a brillo issue to me. 

It allocates a vector size of length. Then resizes it to (length - 1) and checks  [length -1] element.
But the test clearly states this is intentional and assumes that resize() down by one will not re-allocate the memory.
So I am not sure what is right fix here, should I disable asan by adding __attribute__((no_sanitize("address"))).

TEST_F(SecureBlobTest, ResizeTest) {
  // Check that resizing a SecureBlob wipes the excess memory.  The test assumes
  // that resize() down by one will not re-allocate the memory, so the last byte
  // will still be part of the SecureBlob's allocation
  size_t length = 1024;
  SecureBlob blob(length);
  void* original_data = blob.data();
  for (size_t i = 0; i < length; i++) {
    blob[i] = i;
  }

  blob.resize(length - 1);

  EXPECT_EQ(original_data, blob.data());
  EXPECT_EQ(length - 1, blob.size()); // resize to length -1
  EXPECT_EQ(0, blob.data()[length - 1]); // Accessing [length -1] is  oob

Cc: avakulenko@chromium.org dkrahn@chromium.org
+ drahn@ avakulenko@

Comment 5 by vapier@chromium.org, Jan 26 2018

adding the attribute to this func might be our best route here since we specifically want to poke old/unallocated memory

if we were to call resize to increase the memory range again, the API is such that it'll automatically insert a default value in the new location
Cc: warx@chromium.org shapiroc@chromium.org malaykeshav@chromium.org bmgordon@chromium.org
 Issue 806072  has been merged into this issue.
Cc: euge...@chromium.org
Struggling to add attribute since TEST_F macro expands to a class.
Pulling the offending code in a a separate function still triggers asan error.

__attribute__((no_sanitize("address"))) __attribute__((noinline)) unsigned char GetElementAt(const SecureBlob& blob, size_t index) {
  return  blob.data()[index];
}

+eugenis@ for suggestions.

Comment 8 by vapier@chromium.org, Jan 26 2018

to get the builder green, how about disabling the test entirely when asan is enabled ?  there should be a CPP define you could check.

Comment 9 by euge...@google.com, Jan 26 2018

#7: this is really strange. Is that an error in no_sanitize("address") function?
#8: that's probably the easiest
Just as a heads up, fixing libbrillo allowed more packages to be asan tested and some more packages failed in my local testing.

Let's see if the builder also shows those fails after libbrillo fix goes in.
Cc: marcochen@chromium.org
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/external/libbrillo/+/cd5ebb24e6c74e3f64eb0d3877ecb75473b2e96f

commit cd5ebb24e6c74e3f64eb0d3877ecb75473b2e96f
Author: Manoj Gupta <manojgupta@google.com>
Date: Sat Jan 27 04:54:13 2018

libbrillo: Disable ResizeTest when asan is used.

ResizeTest intentationally does an oob access in vector.
Asan complains about this so just disable the test when
asan is used.

BUG= chromium:806013 
TEST=libbrillo unit tests pass with asan.

Change-Id: I6abc2ffa30f8ffc6ecb64d89c7857f584d0ec9af
Reviewed-on: https://chromium-review.googlesource.com/889659
Commit-Ready: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/cd5ebb24e6c74e3f64eb0d3877ecb75473b2e96f/brillo/secure_blob_unittest.cc

Status: Verified (was: Untriaged)
libbrillo error is fixed. But There are more errors now that should be triaged separately.
Labels: libcxx
Labels: libcxx_asan

Sign in to add a comment