--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/e2ba8951-4aac-4150-bb24-936320ee95ean%40chromium.org.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-dev+unsubscribe@chromium.org.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/e2ba8951-4aac-4150-bb24-936320ee95ean%40chromium.org.
--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/b5c0769f-b1eb-4cbe-9240-455e9cf38ad4n%40chromium.org.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-dev+unsubscribe@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/e2ba8951-4aac-4150-bb24-936320ee95ean%40chromium.org.
--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-dev+unsubscribe@chromium.org.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/e2ba8951-4aac-4150-bb24-936320ee95ean%40chromium.org.
--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/b5c0769f-b1eb-4cbe-9240-455e9cf38ad4n%40chromium.org.
--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/3d970704-5320-4122-81fb-08c5cd1a76a2n%40chromium.org.
Are there any benchmarks that cover this performance-critical V8 path? Was there an LLVM bug filed for false positives in UBSan here?
Sounds like we should make the V8 check to be less broad, given that UBSan in LLVM is documented to have a mode that's meant for use in production: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#minimal-runtime
The comment mentions `-fsanitize=bounds`, but unfortunately that was also written to be too broad. `-fsanitize=bounds` is documented to be a combination of two checks, `-fsanitize=array-bounds` and `-fsanitize=local-bounds`. It is probably local-bounds as array-bounds would not see anything relating to malloc sizes.It's a bit unfortunate that there isn't a finer-grained __has_feature (something to ask LLVM folks for?), but given that sanitize=local-bounds isn't even enabled by default in UBSan (see docs), it's probably sufficient to just key it on some #define that the build sets when enabling the feature.
On Mon, Jul 7, 2025 at 9:30 AM Kalvin Lee <kd...@chromium.org> wrote:I had a vain hope that we could fix this forward, but I've prepared a revert CL.
On Mon, Jul 7, 2025 at 3:33 PM David Benjamin <davi...@chromium.org> wrote:Are there any benchmarks that cover this performance-critical V8 path? Was there an LLVM bug filed for false positives in UBSan here?Whether performance is affected depends on whether realloc is cheap in this case. None of this is known because this was only ever meant to be used in non-production builds.
Sounds like we should make the V8 check to be less broad, given that UBSan in LLVM is documented to have a mode that's meant for use in production: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#minimal-runtimeThis just enabled almost every build to think that it's running with ubsan which is not what we expect to run in production.This is basically the same check that is used to detect asan/tsan/msan/ubsan modes. The fact that clang thinks it's a cheap imply that downstream puts only cheap stuff behind these checks.The comment mentions `-fsanitize=bounds`, but unfortunately that was also written to be too broad. `-fsanitize=bounds` is documented to be a combination of two checks, `-fsanitize=array-bounds` and `-fsanitize=local-bounds`. It is probably local-bounds as array-bounds would not see anything relating to malloc sizes.It's a bit unfortunate that there isn't a finer-grained __has_feature (something to ask LLVM folks for?), but given that sanitize=local-bounds isn't even enabled by default in UBSan (see docs), it's probably sufficient to just key it on some #define that the build sets when enabling the feature.I think it would be enough to have a better ubsan check, yes. Do you know something we could use here?
On Mon, Jul 7, 2025 at 10:00 AM Michael Lippautz <mlip...@chromium.org> wrote:On Mon, Jul 7, 2025 at 3:33 PM David Benjamin <davi...@chromium.org> wrote:Are there any benchmarks that cover this performance-critical V8 path? Was there an LLVM bug filed for false positives in UBSan here?Whether performance is affected depends on whether realloc is cheap in this case. None of this is known because this was only ever meant to be used in non-production builds.I'm asking whether there is an overall benchmark that is sensitive to this operation. If it's performance-critical, then I assume there are some important workloads which would regress if this operation started getting slower.That this regression didn't show up in benchmarks suggests that either:- We have a gap in our benchmarks somewhere and should fix that- Our intuition about realloc perf here is wrong and actually it doesn't matter
(Presumably that realloc is a no-op since the usable size is already big enough, so we're really just talking about the costs of dipping into the malloc implementation to look up the usable size a second time within realloc. Maybe that's actually not that expensive. We would need a benchmark to tell.)
ConclusionThe survey of the landscape below makes me think1) We should have a way to make it so production enabled ubsan doesn't trigger __has_feature(undefined_memory_sanitizer), by contributing a patch upstream, adding a new way to detect production undefined memory checks.
2) I think we should fix forward initially and see the perf bot impacts (for angle and V8 if any) initially but if we can't resolve 1 & 3 before the branch point for M140 (Aug 4th) we should revert. There are cases for example in third_party/highway where they do more intensive validation in DEBUG builds, and given third_party/highway is a very specific library I'm wary of suddenly shipping debug marked builds further than Canary/Dev which is likely lacking in testing/perf coverage.
Also that the no_sanitize attribute may be a better way to suppress UBSan warnings, be they false or true positives.
As for the general pattern, there's a lot less of this in non-test code than it seems, actually! Other than the sole example in V8, which turns out to be a true positive and needs fixing anyway, it looks like it's just ANGLE to worry about.
https://source.chromium.org/search?q=__has_feature%5C(undefined_behavior_sanitizer%5C)%20-file:third_party.*third_party%20-file:test&sq=&ss=chromium%2Fchromium%2Fsrchttps://source.chromium.org/search?q=V8_USE_UNDEFINED_BEHAVIOR_SANITIZER%20-file:test&ss=chromium%2Fchromium%2Fsrc
There's a bunch of it in Chromium proper, but that's all based on a build-level define, and not __has_feature, so it wouldn't be affected by this:
https://source.chromium.org/chromium/chromium/src/+/main:build/config/BUILD.gn;drc=82a27169148052767b00c89f2839b35c9913b389;l=95On Mon, Jul 7, 2025 at 2:39 PM David Benjamin <davi...@chromium.org> wrote:I consulted with some compiler folks and they pointed out this was not a false positive. V8 actually needs to be running that realloc across the board, not just conditioned by UBSan:
https://man7.org/linux/man-pages/man3/malloc_usable_size.3.html> This function is intended to only be used for diagnostics and> statistics; writing to the excess memory without first calling
> realloc(3) to resize the allocation is not supported. The
> returned value is only valid at the time of the call.
So actually you need to call realloc to safely write to that anyway.
On Tue, Jul 8, 2025 at 3:25 AM Michael Lippautz <mlip...@chromium.org> wrote:
On Mon, Jul 7, 2025 at 3:06 PM David Benjamin <davi...@chromium.org> wrote:Compiler folks also suggest __size_returning_new as a better version of this API pattern, though it's definitely trickier to use in a context where we don't know the malloc implementation.
https://google.github.io/tcmalloc/reference.html#extensionsI am not sure this is supported by ParitionAlloc; at least I don't see a hint that it is.If it would be available, even just on PA, we'd switch to that conditionally.
I mean, we (Chrome) also own PA, so let's just add it on PA then. That would avoid all this mess. It would probably be better for V8 anyway, Right now you have to get a pointer from PA, then go back and ask PA to look up the size. With a size-returning malloc, they're both returned together and there's no double-lookup.
On Mon, Jul 7, 2025 at 2:39 PM David Benjamin <davi...@chromium.org> wrote:I consulted with some compiler folks and they pointed out this was not a false positive. V8 actually needs to be running that realloc across the board, not just conditioned by UBSan:
https://man7.org/linux/man-pages/man3/malloc_usable_size.3.html> This function is intended to only be used for diagnostics and> statistics; writing to the excess memory without first calling
> realloc(3) to resize the allocation is not supported. The
> returned value is only valid at the time of the call.
So actually you need to call realloc to safely write to that anyway.We are running with PartitionAlloc for builds in Chrome where we have full control over the stack. That man page doesn't really apply here.That said, I think this is all moot as this should be optimized away anyways with PGO and I am happy to check that.
Does V8 as a library ever use something other than PA? It seems MallocUsableSize has branches for Windows _msize and macOS malloc_size. If the optimization is active with other malloc implementations, it ought to be correct there too, even if it doesn't directly impact Chrome.David
One of the reasons ANGLE checks for "__has_feature(undefined_memory_sanitizer)" is that we are using 3rd party tests (dEQP), which have UB.So, we skip the bad tests when we run with UBSan.Having some UBs detected and some not detected makes this confusing.Since we want to opt-out standalone ANGLE from "-fsanitize=array-bounds" anyway, this won't be a problem in practice, though.But for the sake of completeness, if UBSan allows parts of it enabled and parts disabled, it would make sense to have somehting like "__has_feature()" for every UBSan check that can be enabled individually.
On Mon, Jul 7, 2025 at 2:39 PM David Benjamin <davi...@chromium.org> wrote:I consulted with some compiler folks and they pointed out this was not a false positive. V8 actually needs to be running that realloc across the board, not just conditioned by UBSan:
https://man7.org/linux/man-pages/man3/malloc_usable_size.3.html> This function is intended to only be used for diagnostics and> statistics; writing to the excess memory without first calling
> realloc(3) to resize the allocation is not supported. The
> returned value is only valid at the time of the call.
So actually you need to call realloc to safely write to that anyway.We are running with PartitionAlloc for builds in Chrome where we have full control over the stack. That man page doesn't really apply here.That said, I think this is all moot as this should be optimized away anyways with PGO and I am happy to check that.Does V8 as a library ever use something other than PA? It seems MallocUsableSize has branches for Windows _msize and macOS malloc_size. If the optimization is active with other malloc implementations, it ought to be correct there too, even if it doesn't directly impact Chrome.
David