Briefly: `-fsanitize=array-bounds`

139 views
Skip to first unread message

Kalvin Lee

unread,
Jun 10, 2025, 3:47:56 AMJun 10
to memory-safety-dev
We have a write-up on the CL and ensuing discussions here. In short,
Cheers -
Kalvin

Stephen Nusko

unread,
Jun 16, 2025, 3:06:22 AMJun 16
to Kalvin Lee, memory-safety-dev
Thanks so much for looking into this Kalvin!

I think adding a GN opt out for standalone angle is fine, this shouldn't have a high risk of introducing any issues because as a runtime feature crashes that Chrome notice are reportable to angle as security issues for them to fix in their main unhardened code, and it doesn't introduce compile time warnings/errors so there isn't a risk of diverging builds that might fail. Seems like Nico has already OK'd that plan.

Do you have a bug where we can follow along with any open discussions? and see when they get approved/progresses? Sounds like the doc got updated with GPU results, but I only noticed when I went looking today and would like to just get a cc :)

Looking forward to seeing this land.

Cheers,
Stephen



--
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.

Kalvin Lee

unread,
Jun 16, 2025, 3:36:13 AMJun 16
to Stephen Nusko, memory-safety-dev
I don't have a single mega-thread (every team has their own list, and not all are available on the Chromium.org domain), but the results are summarized in the doc. I intended to use this thread for any big announcements, and there aren't any, save that nobody has strongly opposed this in their respective demesnes.

Unless somebody (soon) bursts dramatically into the chapel, I'll be moving to land this CL soon.

Kalvin Lee

unread,
Jul 3, 2025, 1:11:43 AMJul 3
to memory-safety-dev, Kalvin Lee, memory-safety-dev
After fixing up a failing test suite, we've landed the first CL in the sequence. I'll spread the word so that perf-sensitive folks will know and can monitor. The ANGLE escape hatch is ready (for standalone ANGLE), and I just need to prepare a CL to let Chromium interact with it.

To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-dev+unsubscribe@chromium.org.

Kalvin Lee

unread,
Jul 7, 2025, 4:09:38 AMJul 7
to memory-safety-dev

Kalvin Lee

unread,
Jul 7, 2025, 4:14:32 AMJul 7
to memory-safety-dev
(Erratum: the V8 error is runtime, not compile-time.)

Michael Lippautz

unread,
Jul 7, 2025, 6:30:35 AMJul 7
to Kalvin Lee, memory-safety-dev
It is very surprising that this feature now basically makes us enable ubsan in production. A quick check yields at least one example where this introduces a realloc in a performance-critical path used by V8.

What were the follow ups here? Were there any bugs filed? This is suprising behavior and should be fixed asap or reverted.


To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.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.

Kalvin Lee

unread,
Jul 7, 2025, 9:30:14 AMJul 7
to memory-safety-dev, Michael Lippautz
I had a vain hope that we could fix this forward, but I've prepared a revert CL.

To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-dev+unsubscribe@chromium.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.

David Benjamin

unread,
Jul 7, 2025, 9:33:18 AMJul 7
to Kalvin Lee, memory-safety-dev, Michael Lippautz
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.

To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.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.

--
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.

Michael Lippautz

unread,
Jul 7, 2025, 10:00:15 AMJul 7
to David Benjamin, Kalvin Lee, memory-safety-dev, Michael Lippautz
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-runtime


This 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 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.


See above: I think it would be enough if we can get a better check for ubsan mode.

David Benjamin

unread,
Jul 7, 2025, 11:03:35 AMJul 7
to Michael Lippautz, Kalvin Lee, memory-safety-dev
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.)
 
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


This 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?

Like I said, my suggestion was that we make our build, in GN, set some #define when -fsanitize=local-bounds is enabled, and then V8 can key on that. That does make assumptions about the build, so asking the toolchain folks for something better may be worthwhile, but probably fine for something which is off by default.

We should also file an LLVM bug with a minimal reproducer so they can look at whether they can eliminate this false positive. (Or whether it's a true positive. If the compiler ever optimizes based on the reasoning being tripped here, we need the realloc! Though that would be unfortunate given the purpose behind malloc_usable_size.)

Michael Lippautz

unread,
Jul 7, 2025, 12:19:11 PMJul 7
to David Benjamin, Michael Lippautz, Kalvin Lee, memory-safety-dev
Unrelated to performance: IIUC then every build (except for asan for the V8 sandbox) now thinks it's running with ubsan, right? Maybe this is worth announcing more broadly? I see that Angle folks are surprised as well and it's certainly not what I would expect from a regular production build.

On Mon, Jul 7, 2025 at 5:03 PM David Benjamin <davi...@chromium.org> wrote:
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


AllocateAtLeast() is used by the garbage collector for Worklist which is the primitive for young and old generation GCs to collect references. This is used everywhere all the time. 

As for larger benchmarks, this will be visible in `Segment::Create()`  as measured by JetStream and Speedometer on flamegraphs. The benchmarks are generally sensitive to malloc and while overall score may not regress we should still understand the delta here -- see below.

(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.)
 
 
Yeah, this should be a logical nop and my hope is that with PGO it actually just goes away completely.
 
I want to update flamegraphs for go/speedometer-pprof tomorrow on an up-to-date PGO build. Will ping this thread with a profile which should clarify what we can expect.

David Benjamin

unread,
Jul 7, 2025, 2:39:32 PMJul 7
to Michael Lippautz, Kalvin Lee, memory-safety-dev
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.

David Benjamin

unread,
Jul 7, 2025, 3:06:50 PMJul 7
to Michael Lippautz, Kalvin Lee, memory-safety-dev
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#extensions

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%2Fsrc

David Benjamin

unread,
Jul 7, 2025, 5:03:25 PMJul 7
to Michael Lippautz, Kalvin Lee, memory-safety-dev

Stephen Nusko

unread,
Jul 7, 2025, 11:03:22 PMJul 7
to David Benjamin, David Benjamin, Geoff Lang, Michael Lippautz, Kalvin Lee, memory-safety-dev, Yuly Novikov, Jonathan Ross
cc some angle folks for their thoughts: +Jonathan Ross +Yuly Novikov +Geoff Lang 

Conclusion
The survey of the landscape below makes me think

1) 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.
3) We should collect the list of usages and reach out to library owners to create workarounds/fix issues, Chrome's usage of an explicit build define seems a much more sensible way to monitor for ubsan given ubsan is allowed to be used in production. If we can do this before Aug 4th given the few places in code that are doing this we can ship this, if not we can aim for M141 afterwards.

Thoughts?

Rational:

I think there are two levels here in this discussion.

1) High level, detecting ubsan for debug vs production
Is detecting ubsan something that is particularly useful? It appears when it was first requested in 2017 they resisted adding it because unlike other sanitizers it is just a collection of different checks that points out the undefined behaviour, and it is mostly independent of each other. It should be noted that GCC does not provide a way to tell if any undefined behaviour sanitizers are even enabled. Obviously Chrome compiles with Clang so we can tell, but I think it speaks to the potentially lack of usefulness. Indeed in most of those searches above folks are using it only to decide if they add the attribute "no_sanitizer" or suppress certain errors (which I would suppose they could do regardless of ubsan being enabled).

However, given Hyrum's Law, I think we can safely say this is a bit unfortunate, but not technically incorrect. The API exists to tell if any ubsan check is included and people will use it for various things and purposes. I'm unsure if they would accept a change of the definition to only be true if a wider swath of checks are true this seems breaking especially for the usecases of adding attributes to suppress warnings. I do wonder if perhaps they would accept an override where you can say `-fsanitize-production` and this would not trigger the __has_feature(undefined_memory_sanitizer) and instead could be checked with __has_feature(undefined_memory_production_checks) or something similar.

@David Benjamin do you think this might be reasonable to compiler folks? I think it is surprising that now production builds say it is a sanitizer, which historically people assumed was only debugging and while I think most cases shouldn't be conditioning on undefined_bahaviour it seems this is the case already, so it might behove us to make it explicit that sanitizers are running in production. If this is reasonable let's file a bug and get their approval and send a patch to add a new flag in whatever format they desire. We can then enable/disable `-fsanitize-production` based on if we want people acting like we're in a debug sanitizer build.

------------------------------------
However I'd like to also look at the tactical side of things a bit. For more immediate action (should we continue, fix forward, revert, etc).

2) V8 tactical usage.
The two usages in V8, are either 1 the Realloc case and the Linux only V8 sandbox testing API.

As David already beat me to it, this is actually a true positive, it is incorrect to use that memory without a realloc, and in Chrome at least with PartitionAlloc that realloc is relatively cheap (some metadata updates and a return). I think we should just remove that check and always realloc if V8 wants to be able to use that memory.

The other is linux only sandbox testing mode, but I think crashing in the face of bounds checking code is incorrect and ubsan never has callbacks (like what they install for address sanitizer), so likely they just shouldn't crash in that case we can remove this check (a fix forward)

3) Other usage

Angle seems to use it primarily just to check in tests and to add additional ubsan config, they have a plan to disable it in their library which fixes their roll temporarily, other usages are like third_party/highway (making it a debug build), or jdhuff.c code for example just uses it to add no_sanitize attributes for overflow, I wonder why they don't always add the attributes? They obviously don't add it for GCC, they add it only if they detect some ubsan when in reality I would assume their function always contains the overflow and should just probably unconditionally add the attribute.

Cheers,
Stephen

Michael Lippautz

unread,
Jul 8, 2025, 3:25:45 AMJul 8
to Stephen Nusko, David Benjamin, David Benjamin, Geoff Lang, Michael Lippautz, Kalvin Lee, memory-safety-dev, Yuly Novikov, Jonathan Ross
On Tue, Jul 8, 2025 at 5:03 AM Stephen Nusko <nus...@chromium.org> wrote:
cc some angle folks for their thoughts: +Jonathan Ross +Yuly Novikov +Geoff Lang 

Conclusion
The survey of the landscape below makes me think

1) 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.

For V8: If you can forward fix ASAP then go ahead. Otherwise, we need to revert as this is blocking the DEPS roll which is not optional.
I 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.
 
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%2Fsrc

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.

David Benjamin

unread,
Jul 8, 2025, 1:53:07 PMJul 8
to Michael Lippautz, Stephen Nusko, Geoff Lang, Kalvin Lee, memory-safety-dev, Yuly Novikov, Jonathan Ross
(Resending from the correct address.)

On Tue, Jul 8, 2025 at 1:51 PM David Benjamin <davi...@google.com> wrote:
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#extensions


I 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

David Benjamin

unread,
Jul 8, 2025, 4:34:08 PMJul 8
to Yuly Novikov, Michael Lippautz, Stephen Nusko, Geoff Lang, Kalvin Lee, memory-safety-dev, Jonathan Ross
I don't suppose dEQP is something we can just fix? If there's UB in it, it can also affect non-UBSan builds in the form of compilers making assumptions. Seems worth fixing anyway.

Alternatively, rather than skipping the bad tests, what if we just filled in suppressions for the bad parts of dEQP?

As a bonus, that would also document the individual problems, so folks can go in and fix them.

On Tue, Jul 8, 2025 at 3:56 PM Yuly Novikov <ynov...@google.com> wrote:
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.

Kalvin Lee

unread,
Jul 9, 2025, 12:06:44 AMJul 9
to memory-safety-dev, David Benjamin, Michael Lippautz, Stephen Nusko, Geoff Lang, Jonathan Ross, Yuly Novikov

Michael Lippautz

unread,
Jul 9, 2025, 4:25:20 AMJul 9
to Kalvin Lee, memory-safety-dev, David Benjamin, Michael Lippautz, Stephen Nusko, Geoff Lang, Jonathan Ross, Yuly Novikov
The V8 sandbox issue should be fixed now.

Yes, that indeed would be much better. Filed an FR at https://crbug.com/430336828.
 

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.


V8 can and is embedded elsewhere with different malloc implementations. The callsites are used everywhere (GC, pretty much) and we never heard any complaints about our AllocateAtLeast.
 
David
Reply all
Reply to author
Forward
0 new messages