New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users
Status: Verified
Owner:
Closed: Jan 27
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 Back to list
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.
 
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@
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.
to get the builder green, how about disabling the test entirely when asan is enabled ?  there should be a CPP define you could check.
#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
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
libbrillo error is fixed. But There are more errors now that should be triaged separately.
Labels: libcxx
Sign in to add a comment