Closed
Bug 810823
Opened 12 years ago
Closed 12 years ago
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'
Categories
(Core :: Graphics: Text, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: decoder, Unassigned)
References
Details
(Keywords: sec-low, Whiteboard: [-fsanitize=object-size])
Attachments
(2 files, 2 obsolete files)
2.99 KB,
patch
|
decoder
:
feedback+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
decoder
:
feedback+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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...
Attachment #680611 -
Flags: review?(mozilla)
Reporter | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
(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...
Reporter | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #680645 -
Flags: review?(mozilla)
Updated•12 years ago
|
Attachment #680611 -
Attachment is obsolete: true
Attachment #680611 -
Flags: review?(mozilla)
Comment 8•12 years ago
|
||
Huh, wait a sec, I don't think that patch will work either.... back to the macros....
Comment 9•12 years ago
|
||
OK, I think this version should actually work.
Attachment #680650 -
Flags: review?(mozilla)
Updated•12 years ago
|
Attachment #680645 -
Attachment is obsolete: true
Attachment #680645 -
Flags: review?(mozilla)
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 680650 [details] [diff] [review]
ensure sufficient data is provided for Null records
Fix confirmed, thanks for unblocking :)
Attachment #680650 -
Flags: feedback+
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
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.)
Attachment #680668 -
Attachment is patch: true
Attachment #680668 -
Attachment mime type: text/x-c → text/plain
Reporter | ||
Comment 13•12 years ago
|
||
Comment on attachment 680668 [details] [diff] [review]
Alternative patch Jonathan and I came up with
Confirmed that this works as well :)
Attachment #680668 -
Flags: feedback+
Comment 14•12 years ago
|
||
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.
Attachment #680650 -
Flags: review?(mozilla)
Comment 15•12 years ago
|
||
Fixed on trunk by bug 801410, which included the (second) patch here as part of the new upstream version.
Updated•12 years ago
|
Updated•12 years ago
|
status-firefox-esr10:
--- → wontfix
status-firefox-esr17:
--- → wontfix
Updated•12 years ago
|
status-b2g18:
--- → wontfix
Updated•11 years ago
|
Whiteboard: [-fsanitize=object-size]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•