On Mon, 13 Feb 2023 16:57:17 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:
> The `jimage` location attributes are terminated with `ATTRIBUTE_END`-kinds. > However, > the byte containing `ATTRIBUTE_END` (most significant 5 bits, represent > `kind`), might > be non-zero in the lower 3 bits (values up to `0x07` represent > `ATTRIBUTE_END`). The JDK code > handles this case correctly in > [`ImageLocation.decompress()`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java#L69..L71). > However, the `libjimage` > code in `java.base` doesn't. That can lead to segfaults reading random bytes > and offsets. > > I propose to break the loop if `ATTRIBUTE_END` is being encountered so that > reading stops. > > Testing: > - [x] `test/jdk/tools/jimage` and `test/jdk/jdk/internal/jimage` tests. > - [x] Manual testing with a patched JDK to write non-zero bytes containing > `ATTRIBUTE_END` into the jimage. Segfaults before, runs fine after. > - [ ] GHA. In progress. > > Thoughts? Your patch looks fine to me. Actually slightly clearer to the casual reader than the comparison <=7 the JDK does. The (preexisting) loop condition is weird; when would data ever become NULL? src/java.base/share/native/libjimage/imageFile.cpp line 132: > 130: // Extract kind from header byte. > 131: u1 kind = attribute_kind(byte); > 132: assert(kind < ATTRIBUTE_COUNT && "invalid image location > attribute"); Not your patch, but the assert is superfluous since attribute_kind already asserts. ------------- Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/12539
