[compiler-rt][ASan] Remove alignment message in ASan error reporting

[AdvenamTacet]

This commit removes unnecessary alignment check and message in ASan error reporting functions (like ErrorBadParamsToAnnotateContiguousContainer::Print()), as alignment is no longer required starting from LLVM 16.

Without that commit, this message can be observed only when arguments are truly incorrect and beg is unaligned. Just unaligned beg does not result in any message being printed.

Related commits:

• dd1b7b7
• 1c5ad6d

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Tacet (AdvenamTacet)

Changes

Removed unnecessary alignment checks in ASan error reporting functions, as alignment is no longer required starting from LLVM 16.

Related commits:

• dd1b7b7
• 1c5ad6d

---

Full diff: https://github.com/llvm/llvm-project/pull/94103.diff

1 Files Affected:

• (modified) compiler-rt/lib/asan/asan_errors.cpp (-4)

diff --git a/compiler-rt/lib/asan/asan_errors.cpp b/compiler-rt/lib/asan/asan_errors.cpp
index 3f2d13e314640..26eabf2400b77 100644
--- a/compiler-rt/lib/asan/asan_errors.cpp
+++ b/compiler-rt/lib/asan/asan_errors.cpp
@@ -328,8 +328,6 @@ void ErrorBadParamsToAnnotateContiguousContainer::Print() {
       "      new_mid : %p\n",
       (void *)beg, (void *)end, (void *)old_mid, (void *)new_mid);
   uptr granularity = ASAN_SHADOW_GRANULARITY;
-  if (!IsAligned(beg, granularity))
-    Report("ERROR: beg is not aligned by %zu\n", granularity);
   stack->Print();
   ReportErrorSummary(scariness.GetDescription(), stack);
 }
@@ -348,8 +346,6 @@ void ErrorBadParamsToAnnotateDoubleEndedContiguousContainer::Print() {
       (void *)old_container_end, (void *)new_container_beg,
       (void *)new_container_end);
   uptr granularity = ASAN_SHADOW_GRANULARITY;
-  if (!IsAligned(storage_beg, granularity))
-    Report("ERROR: storage_beg is not aligned by %zu\n", granularity);
   stack->Print();
   ReportErrorSummary(scariness.GetDescription(), stack);
 }

[vitalybuka]

Would you mind to create a test to hit this check?

[vitalybuka]

Test if possible
or explanation if it's hard

[AdvenamTacet]

This week I have no access to my computer, but I have reserved some time next week for open source related tasks and I will look at it.

It shouldn't be a problem to create an example, incorrect arguments (eg. end < beg) & unaligned beg should print it.

But this message is simply obsolete, even if impossible to trigger. Unaligned buffers are supported.

[AdvenamTacet]

Example showing that __sanitizer_annotate_contiguous_container works with unaligned buffers without printing any message: godbolt

#include <sanitizer/asan_interface.h>
#include <iostream>

constexpr size_t N = 64;
constexpr size_t off = 1;

int main() {

    char buffer[N + off];
    char *beg = buffer + off;
    char *end = beg + N;

    __sanitizer_annotate_contiguous_container(beg, end, end, beg);
    std::cout << beg[0] << std::endl;
}

This example correctly detects container overflow (whole buffer is poisoned).

=>0x7a1ccb000000: f1 f1 f1 f1[01]fc fc fc fc fc fc fc fc f3 f3 f

To get this message, it's enough to provide incorrect arguments to the function, for example providing old_mid outside of the container, like __sanitizer_annotate_contiguous_container(beg, end, end+1, beg);.
Then we get:

==1==ERROR: AddressSanitizer: bad parameters to __sanitizer_annotate_contiguous_container:
      beg     : 0x74ab7b600021
      end     : 0x74ab7b600061
      old_mid : 0x74ab7b600062
      new_mid : 0x74ab7b600021
==1==ERROR: beg is not aligned by 8

Clearly beg not being aligned by 8 is not a problem here.

Edit:
The same is true for double ended contiguous containers.

• Detecting overflows example for double ended containers.
• Example with the message for __sanitizer_annotate_double_ended_contiguous_container.

---

Those are only two lines with that error message.

$ grep -R -I -H "is not aligned by"
compiler-rt/lib/asan/asan_errors.cpp:    Report("ERROR: beg is not aligned by %zu\n", granularity);
compiler-rt/lib/asan/asan_errors.cpp:    Report("ERROR: storage_beg is not aligned by %zu\n", granularity);
llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test:## PAddr is not aligned by sh_addralign(.data)
$

---

@vitalybuka do you think adding anything to that PR makes sense? If yes, what test do you have in mind?

[vitalybuka]

#include <sanitizer/asan_interface.h>
#include <iostream>

constexpr size_t N = 64;
constexpr size_t off = 1;

int main() {

    char buffer[N + off];
    char *beg = buffer + off;
    char *end = beg + N;

    __sanitizer_annotate_contiguous_container(beg, end, end, beg);
}

Just add above as a test, it should exit with 0?

[AdvenamTacet]

Yes, it should exit with zero. I can add it, but we test it in contiguous_container.cpp test already.

---

Creating unaligned beg:

llvm-project/compiler-rt/test/asan/TestCases/contiguous_container.cpp

Line 123 in 055e431

char *st_beg = buffer + off_begin;

Using unaligned beg:

llvm-project/compiler-rt/test/asan/TestCases/contiguous_container.cpp

Lines 137 to 138 in 055e431

	__sanitizer_annotate_double_ended_contiguous_container(
	st_beg, st_end, old_beg, old_end, beg, end);

---

Same for __sanitizer_annotate_contiguous_container:

llvm-project/compiler-rt/test/asan/TestCases/contiguous_container.cpp

Lines 49 to 59 in 055e431

	char *st_beg = buffer + off_begin;
	char *st_end = st_beg + capacity;
	char *end = poison_buffer ? st_beg : st_end;
	for (int i = 0; i < 1000; i++) {
	size_t size = rand() % (capacity + 1);
	assert(size <= capacity);
	char *old_end = end;
	end = st_beg + size;
	__sanitizer_annotate_contiguous_container(st_beg, st_end, old_end, end);

[vitalybuka]

Yes, it should exit with zero. I can add it, but we test it in contiguous_container.cpp test already.

I am confused. Then why they are not triggering these messages without your patch?

[AdvenamTacet]

This patch here only removes printing that part of the message, which is not printed for unaligned beg.

Whenever incorrect arguments are provided ErrorBadParamsToAnnotateContiguousContainer::Print() is called, and all arguments are printed etc.
Additionally, inside that Print, if beg is unaligned, that message is printed.

However, because unaligned beg is correct, it does not trigger any message. The message is printed if another argument is incorrect and additionally beg is not aligned (and therefore is confusing).

Edit: to clarify, we print "bad params error" in correct moments, just we should not mention unaligned beg as a problem.

[vitalybuka]

Never mind.
You don't have a test. That one godbolt does not trigger messages (on the HEAD).

[AdvenamTacet]

Now I am confused. On trunk we can see the message, but at least one argument must be truly incorrect, same with older LLVM versions.

What we can do is add a check to crash test.

void BadBoundsUnaligned() {
  long t[100];
// CHECK-BAD-BOUNDS-UNALIGNED: ERROR: AddressSanitizer: bad parameters to __sanitizer_annotate_contiguous_container
// CHECK-BAD-BOUNDS-UNALIGNED: <rule telling that "beg is not aligned by" does not appear in the error message>
  __sanitizer_annotate_contiguous_container(&t[1], &t[0] + 100, &t[0] + 101,
                                            &t[0] + 50);
}

To confirm that this message does not contain "beg is not aligned by".

If you think it makes sense, I will add it.

But, if arguments are correct (including unaligned beg), nothing is printed.

[vitalybuka]

Now I am confused. On trunk we can see the message, but at least one argument must be truly incorrect, same with older LLVM versions.

Sorry, I clicked the first https://godbolt.org/ link which was messaging on 16 only, and forgot that it had instruction in the comment.

What we can do is add a check to crash test.

void BadBoundsUnaligned() {
  long t[100];
// CHECK-BAD-BOUNDS-UNALIGNED: ERROR: AddressSanitizer: bad parameters to __sanitizer_annotate_contiguous_container
// CHECK-BAD-BOUNDS-UNALIGNED: <rule telling that "beg is not aligned by" does not appear in the error message>
  __sanitizer_annotate_contiguous_container(&t[1], &t[0] + 100, &t[0] + 101,
                                            &t[0] + 50);
}

To confirm that this message does not contain "beg is not aligned by".

If you think it makes sense, I will add it.

Yes, please.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[AdvenamTacet]

@vitalybuka do you know how correctly create "does not appear" rule? I added -NOT-, but it seems to not work.

[vitalybuka]

@vitalybuka do you know how correctly create "does not appear" rule? I added -NOT-, but it seems to not work.

I don't like NOT at all. Note, -NOT must be the suffix.

I like --implicit-check-not="PATTERN"

In your case I'd suggest --implicit-check-not="ERROR: AddressSanitizer"
Than ALL instances of this patter must be matches with some CHECK: statement, or test fail.

[AdvenamTacet]

Ok, I tested this test in #101349 and it works as intended. I used "implicit-check-not", thanks for pointing me into that, I didn't know it.
I'm going to merge it, unless anyone sees something to fix here.

[chapuni]

2x2 warnings :)

[AdvenamTacet]

@chapuni thank you for fixing them!