User Details
- User Since
- Oct 3 2012, 3:00 AM (508 w, 21 h)
Fri, Jun 24
Yep. Ubsan does not detect potential overflows, it detects actual overflows as they happen. The problem is real.
Won't that just make the tests pass, but keep potential integer overflows (UB) in the compiler? Sounds suboptimal.
Thu, Jun 23
This change causes ubsan failures due to integer overflow, see https://lab.llvm.org/buildbot/#/builders/5/builds/25185
Wed, Jun 22
LGTM
The leaks are still there, reverting.
Could you add a test?
LGTM, this is great!
LGTM
Wed, Jun 15
LGTM
LGTM
Tue, Jun 14
LGTM
LGTM
Tue, Jun 7
I don't have a lot of arguments either way, do you?
Ignorelist support is one for the new sanitizer type.
LGTM
Mon, Jun 6
LGTM
You can llvm-objdump --dwarf=frames a .o file.
Should we have a test for the augmentation string, through llvm-readelf?
Typo: "runttime"
The interceptor (dn_expand) is only defined on linux && !android, while this code links libresolv on *bsd and others, too. I think it's ok, as long as all platforms with the interceptor are covered.
Thu, Jun 2
We should also add -lresolv to tools::linkSanitizerRuntimeDeps if we are now intercepting functions from there.
Libresolv is part of libc.so on android.
May 16 2022
Seems fine to me, if fstatat64 is indeed the canonical syscall on sparc.
May 12 2022
May 10 2022
We've had enough problems with fixed mapping that I prefer dynamic shadow address for anything going forward. Also, fixed mapping address becomes an ABI constant, which causes issues with prebuilt libraries.
May 9 2022
There is a range of addresses where the kernel may place the main executable. It has been extended once in the past (ASLR). ASan mapping is a compile-time constant, so it must work with any location within that range. I suspect that for QEMU this range may be different.
LGTM
May 5 2022
LGTM
LGTM
LGTM
May 4 2022
The approach looks fine. Should we also put a <html></html> pair around the output, or do you want to treat it as a snippet?
This script is starting to get complex. Perhaps we should write tests?
LGTM
May 3 2022
I've always viewed the current implementation as a hack, and believed this data should live in debug info.
May 2 2022
Apr 29 2022
Fix internal_fork on s390.
The mappings are defined such that the address range where the kernel might place the main executable image do not overlap with the shadow region. This can be different in qemu.
Apr 28 2022
Thanks! I'll update the patch later. Apparently there is like 5 different signatures...
Thanks. Ulrich, could you help me with the s390 failures? I can reland with a #define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS !SANITIZER_S390 for now, but it would be great to remove the non-canonical code path completely.
Apr 22 2022
Apr 21 2022
.
address comments
address comments
Mild preference for !defined(__ILP32__).
LGTM
Sorry, I did not mean that I was going to implement it.
LGTM, but would it be more idiomatic to check for the _ILP32 macro?
Apr 20 2022
I still think that my proposal in the comment above is way better than adding an ABI-breaking compiler flag.
Apr 19 2022
This breaks ubsan on the bot:
android_compile script is confused by -emit-llvm it looks like. I'd just remove the IR checks from the test.
Apr 18 2022
LGTM w/ nits
Generally, there is no way to tell the target page size in advance. And there is no need for that, too - just untie this value from the page size instead, and fix the munmap issue in the runtime library by up/down aligning the bounds to the runtime page size. You can also increase the default / minimal ring buffer size at runtime to be a multiple of the page size.
Apr 13 2022
LGTM
Apr 12 2022
Could you clarify that the problem is specifically with byval argument in the patch description?
LGTM
Apr 11 2022
LGTM
Apr 8 2022
LGTM
LGTM
LGTM
Apr 7 2022
LGTM
Mar 29 2022
LGTM
Mar 17 2022
LGTM, nice!
Mar 15 2022
- Do you need per-object marking that a linker will aggregate? For example do you enable when at least one object needs MTE, or do you need all objects to be compatible with MTE? A per object marking that can be overridden at link time will give you a chance to diagnose objects that may not have the MTE support. It can help to have an assembler option to fabricate the note if objects are used.
The ELF note is coupled with the executable, not the DSOs, and thus the executable decides what MTE options are enabled. In future, there will be a per-DSO progbits section for MTE global descriptors, but these progbits sections will be ignored if the main executable doesn't include the MTE ELF note. And, of course, MTE stack is enabled on a per-CU level.
Mar 14 2022
Add a comment at the top for ease of understanding. Something like "Test that storage for allocas with disjoint lifetimes is reused with or without stack tagging"
The concept of memtag is not android specific, but the current implementation is. It might grow support for other OS targets in the future.
Mar 10 2022
LGTM
Mar 8 2022
LGTM
Mar 4 2022
LGTM
Mar 3 2022
LGTM
Mar 1 2022
LGTM
Feb 28 2022
LGTM
Feb 23 2022
LGTM
Feb 17 2022
TBH I'm on the fence about whether poison-at-lifetime is an optimization vs "core functionality", but lets go with this change. The important part is not to run the analysis when the function does not have the attribute (that's what the *-pipeline tests check).
Change LGTM but the description does not make any sense to me.