1932412 - Invalid read in IsValidAllocKind (debug only)

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Julian Seward [:jseward]]

I've seen this many times in the past couple of months, both in shell builds
and full browser builds. It only happens in debug builds. I imagine it's
harmless; nevertheless it would be nice if it went away since (1) it injects
noise into the "is this [shell/browser] valgrind-clean?" question, and (2)
unaccounted-for memory accesses are generally disconcerting.

Please declassify as appropriate.

I attach a test case for the shell. The mechanics of the test case are mostly
unimportant. It merely needs to do enough allocation to cause the GC to Do
Stuff. The test case is an allocation-heavy script I happened to have lying
around.

STR: configure m-c on x86_64-linux with

--enable-debug --enable-optimize="-g -Og" --disable-jemalloc --enable-valgrind

Run:

valgrind --sigill-diagnostics=no --progress-interval=10
--show-mismatched-frees=no --fair-sched=yes
./dist/bin/js --wasm-compiler=baseline badness118.js

Complaints and the test case are in subsequent comment(s).

Comment 1

[Julian Seward [:jseward]]

Attachment: badness118.js -- the test case

Comment 2

[Julian Seward [:jseward]]

Attachment: valgrind/memcheck complainage

Comment 3

[Julian Seward [:jseward]]

Oh, I should add: the offending addresses always seem to be
of the form 0x....008, that is, bytes 8 .. 15 in a hardware page.

Comment 4

[Jon Coppeard (:jonco)]

It looks like this is because we're doing a 8 byte read to access a single byte field and it's colliding with another field that's been marked as MemCheckKind::MakeNoAccess. This only happens for an assertion so it's not a problem in release builds.

Fixing this is slightly tricky because the JIT has assumptions about Arena field layout baked into it.

Comment 5

[Julian Seward [:jseward]]

Attachment: bug1932412-IsValidAllocKind.diff-1

I made the attached patch, which tells valgrind not to report address errors in
the afflicted range. It doesn't change the tracked state of the underlying
memory and hence shouldn't affect any other checking that is based on the
tracked state of that memory. It also doesn't change the actual GC code/logic
in any way.

Comment 6

[Julian Seward [:jseward]]

Attachment: valgrind/memcheck complainage, updated to m-c of 2025Apr28

Updated to m-c 784713:2149fbb1f153 of Mon Apr 28 06:56:14 2025 +0000.

Comment 7

[Julian Seward [:jseward]]

Attachment: Bug 1932412 - Invalid read in IsValidAllocKind (debug only). r=jonco.

In debug builds, in js::gc::Arena::allocated, valgrind complains about the
access to allocKind even though it is legitimate, as a result of earlier
client requests to mark the area as no-access. This patch makes those errors
disappear by temporarily disabling reporting of addressing errors in that
range.

Comment 8

[Julian Seward [:jseward]]

Declassifying, in consultation with Jon. This isn't a sec- bug at all -- there
is no invalid memory access. It's merely an artefact of the way in which the
access interacts with existing valgrind address-annotation code.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd3e443cec19
Invalid read in IsValidAllocKind (debug only).  r=jonco.

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/fd3e443cec19