If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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'

RESOLVED FIXED

Status

()

Core
Graphics: Text
--
major
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: decoder, Unassigned)

Tracking

({sec-low})

Trunk
x86_64
Linux
sec-low
Points:
---

Firefox Tracking Flags

(firefox-esr10 wontfix, firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [-fsanitize=object-size])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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).
Created attachment 680611 [details] [diff] [review]
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...
Attachment #680611 - Flags: review?(mozilla)
(Reporter)

Comment 3

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

5 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?
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.)
Created attachment 680645 [details] [diff] [review]
ensure sufficient data is provided for Null records
Attachment #680645 - Flags: review?(mozilla)
Attachment #680611 - Attachment is obsolete: true
Attachment #680611 - Flags: review?(mozilla)
Huh, wait a sec, I don't think that patch will work either.... back to the macros....
Created attachment 680650 [details] [diff] [review]
ensure sufficient data is provided for Null records

OK, I think this version should actually work.
Attachment #680650 - Flags: review?(mozilla)
Attachment #680645 - Attachment is obsolete: true
Attachment #680645 - Flags: review?(mozilla)
(Reporter)

Comment 10

5 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

5 years ago
Created attachment 680668 [details] [diff] [review]
Alternative patch Jonathan and I came up with
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

5 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 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)
Fixed on trunk by bug 801410, which included the (second) patch here as part of the new upstream version.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Depends on: 801410
Resolution: --- → FIXED
Keywords: sec-want → sec-low
status-firefox-esr10: --- → wontfix
status-firefox-esr17: --- → wontfix

Updated

5 years ago
status-b2g18: --- → wontfix

Updated

4 years ago
Whiteboard: [-fsanitize=object-size]

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.