810823 - gfx/harfbuzz/src/hb-ot-layout-common-private.hh:209:1: fatal error: reference binding to address <addr> with insufficient space for an object of type 'const OT::LangSys'

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Christian Holler (:decoder)]

In a build compiled with ubsan (undefined behavior sanitizer) I'm getting this abort when starting up mochitests. The message points to this line:

DEFINE_NULL_DATA (LangSys, "\0\0\xFF\xFF");

which would expand to

static const char _NullLangSys[LangSys::min_size + 1] = data;
template <>
inline const LangSys& Null<LangSys> (void) {
  return *CastP<LangSys> (_NullLangSys);
}
ASSERT_STATIC (LangSys::min_size + 1 <= sizeof (_NullLangSys))


The message indicates that something is wrong here with pointer sizes/references, but I cannot immediately see it as I'm not familiar with the code. It would be very nice if someone could quickly look into this, because it's blocking further testing (I cannot make the abort non-fatal).

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch, ensure enough data is provided for Null records

It looks to me like the string provided to DEFINE_NULL_DATA needs to be extended to explicitly provide enough data for the type's Null record, both here and for RangeRecord. Otherwise, a reference to <type>::Null will be an object that includes the string's NUL terminator (which would be OK, as it happens), but also whatever byte(s) follow that, which AFAICT are not guaranteed to be zeroed. Behdad, please double-check...

Comment 3

[Christian Holler (:decoder)]

Thanks for the quick response. I tested the fix and unfortunately I'm still getting exactly the same error :( I'll try to expand the macro and see where exactly it is complaining.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Christian Holler (:decoder) from comment #3)
> Thanks for the quick response. I tested the fix and unfortunately I'm still
> getting exactly the same error :(

That's disappointing; I thought it looked nice and easy. :( I guess it's time to try and understand the tangle of macros and types a bit more thoroughly.

Thanks for testing it so promptly, though! Better to know now than later...

Comment 5

[Christian Holler (:decoder)]

I expanded the macro and made some debugging:

(gdb) f 2
#2  0x00007ffff3fb22a5 in Null<OT::LangSys> () at /home/decoder/LangFuzz/mozilla-central-browser/gfx/harfbuzz/src/hb-ot-layout-common-private.hh:213
213               return *CastP<LangSys> (_NullLangSys);
(gdb) p sizeof(LangSys)
$1 = 8
(gdb) p sizeof(_NullLangSys)
$2 = 7

That is likely the problem. When I change the min_size + 1 to min_size + 2 in the expanded macro (of course that's not the fix, but good to confirm it's actually this size it doesn't like), then the error goes away.

Does that help? Is it even correct that it tries to interpret what's *inside* the char[] here as the pointer? Or am I getting something wrong here?

Comment 6

[Jonathan Kew [:jfkthame]]

Ah, I miscounted, sorry - sizeof(LangSys) is indeed 8, not 6 as I thought; the IndexArray type (for featureIndex) will have sizeof = 4, although when its count is zero we'd never access anything beyond the first 6 bytes.

So we need a couple more null bytes on the end of the string. Strictly speaking one more would be enough, as there'll also be the null terminator that C strings carry, but relying on that seems rather hacky.

(The need to extend the string to 8 bytes rather than 6 is really just an artifact of how the structs are defined, and the fact that not all compilers allow a zero-element array as the idiom for a variable-length array in a struct, so we have to declare them as one-element. But I don't think it's worth fighting the compiler over those couple of extra bytes.)

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: ensure sufficient data is provided for Null records

Comment 8

[Jonathan Kew [:jfkthame]]

Huh, wait a sec, I don't think that patch will work either.... back to the macros....

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: ensure sufficient data is provided for Null records

OK, I think this version should actually work.

Comment 10

[Christian Holler (:decoder)]

Comment on attachment 680650 [details] [diff] [review]
ensure sufficient data is provided for Null records

Fix confirmed, thanks for unblocking :)

Comment 11

[Behdad Esfahbod]

Attachment: Alternative patch Jonathan and I came up with

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 680668 [details] [diff] [review]
Alternative patch Jonathan and I came up with

Christian, we believe this should also fix the problem - could you please test it and confirm? Thanks. (Patch applies within gfx/harfbuzz directory.)

Comment 13

[Christian Holler (:decoder)]

Comment on attachment 680668 [details] [diff] [review]
Alternative patch Jonathan and I came up with

Confirmed that this works as well :)

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 680650 [details] [diff] [review]
ensure sufficient data is provided for Null records

We're aiming to take a new harfbuzz version from upstream (in bug 801410) that will include the fix for this issue.

Comment 15

[Jonathan Kew [:jfkthame]]

Fixed on trunk by bug 801410, which included the (second) patch here as part of the new upstream version.