Closed Bug 1315986 Opened 4 years ago Closed 4 years ago

AddressSanitizer: global-buffer-overflow on address 0x at pc 0x bp 0x sp 0x

Categories

(Core :: Layout: Text and Fonts, defect)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 - fixed

People

(Reporter: cbook, Assigned: jfkthame)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files)

Attached file stack
Bughunter reports reports lots of AddressSanitizer: global-buffer-overflow on address 0x at pc 0x bp 0x sp 0x on sites like 

https://twitter.com/search?q=Golden%20Tate&src=tyah
http://www.espn.com/nfl/scoreboard
etc

seems nightly only and graphics related (no idea if related to bug 1315979)

Johnathan, could you take a look, thanks!
Flags: needinfo?(jfkthame)
(In reply to Carsten Book [:Tomcat] from comment #0)
> Created attachment 8808626 [details]
> stack
> 
> Bughunter reports reports lots of AddressSanitizer: global-buffer-overflow
> on address 0x at pc 0x bp 0x sp 0x on sites like 
> 
> https://twitter.com/search?q=Golden%20Tate&src=tyah
> http://www.espn.com/nfl/scoreboard
> etc
> 
> seems nightly only and graphics related (no idea if related to bug 1315979)
> 
> Johnathan, could you take a look, thanks!

Can you cc me on bug 1315979, if there might be a connection there? I can't access that bug...
Flags: needinfo?(jfkthame) → needinfo?(cbook)
tested http://www.espn.com/nfl/scoreboard on a linux trunk asan build from taskcluster and seems that page hung the browser after awhile
Flags: needinfo?(cbook)
i was able to reproduce this also on http://www.thedailymeal.com/recipes/rustic-cheddar-potato-soup-recipe

0x7f4de5ada0a8 is located 56 bytes to the left of global variable 'gPairConservative' defined in '/home/worker/workspace/build/src/intl/lwbrk/nsJISx4051LineBreaker.cpp:280:23' (0x7f4de5ada0e0) of size 24
0x7f4de5ada0a8 is located 0 bytes to the right of global variable 'sUnicodeLineBreakToClass' defined in '/home/worker/workspace/build/src/intl/lwbrk/nsJISx4051LineBreaker.cpp:510:23' (0x7f4de5ada080) of size 40
SUMMARY: AddressSanitizer: global-buffer-overflow (/home/tomcat/asan/opt/firefox/libxul.so+0x1f9aa4e) 
Shadow bytes around the buggy address:
  0x0fea3cb533c0: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 00 00 00
  0x0fea3cb533d0: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9
  0x0fea3cb533e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fea3cb533f0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fea3cb53400: 00 00 00 00 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9
=>0x0fea3cb53410: 00 00 00 00 00[f9]f9 f9 f9 f9 f9 f9 00 00 00 f9
  0x0fea3cb53420: f9 f9 f9 f9 00 00 00 f9 f9 f9 f9 f9 00 00 00 00
  0x0fea3cb53430: 06 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0fea3cb53440: 05 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0fea3cb53450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fea3cb53460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==2227==ABORTING
Flags: needinfo?(jfkthame)
Component: Graphics → Layout: Text
I suspect this may be a regression from bug 1265631, though I haven't spotted what's wrong with the code there yet...
Ah, got it. This is triggered by bug 1299615: the new ICU (and Unicode) version has additional ULineBreak values that are not covered by the sUnicodeLineBreakToClass array.

So we need to extend that array; and we should also add some kind of assertion to detect issues like this in future.
Blocks: 1299615
Flags: needinfo?(jfkthame)
Keywords: regression
[Tracking Requested - why for this release]:
regression - bughunter found
A minimal testcase that will trigger this:

  data:text/html,&%23x270c;

...because U+270C now gets ULineBreak value U_LB_E_BASE, which is new in ICU58 and runs off the end of the sUnicodeLineBreakToClass array that was based on ICU56 property values.

Patch coming...
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8808999 [details] [diff] [review]
Update line-break class mapping in nsJISx4051LineBreaker to handle new classes in ICU58/Unicode 9, and add assertions to detect any future additions that will require further updates

>+    /* ZWJ = 42,                          [ZWJ]*/ CLASS_CHARACTER

Hmm, could be CLASS_NON_BREAKABLE but I'm not sure.

>-  return sUnicodeLineBreakToClass[mozilla::unicode::GetLineBreakClass(u)];
>+  static_assert(U_LB_COUNT == mozilla::ArrayLength(sUnicodeLineBreakToClass),
>+                "Gecko vs ICU LineBreak class mismatch");
>+
>+  auto cls = mozilla::unicode::GetLineBreakClass(u);
>+  MOZ_ASSERT(cls < mozilla::ArrayLength(sUnicodeLineBreakToClass));
>+  return sUnicodeLineBreakToClass[cls];

Nice!
Attachment #8808999 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #9)
> Comment on attachment 8808999 [details] [diff] [review]
> Update line-break class mapping in nsJISx4051LineBreaker to handle new
> classes in ICU58/Unicode 9, and add assertions to detect any future
> additions that will require further updates
> 
> >+    /* ZWJ = 42,                          [ZWJ]*/ CLASS_CHARACTER
> 
> Hmm, could be CLASS_NON_BREAKABLE but I'm not sure.

I believe this is currently handled by the custom tables above anyhow, so it doesn't actually get here, but this is the value GetClass currently returns for ZWJ.

(I suspect maybe we could streamline the earlier part of GetClass and rely more on the Unicode properties, but that should be considered separately; here, I just want to fix the immediate bug.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c17531742f635bcb7bac22ba9e80c23afd7d49b5
Bug 1315986 - Update line-break class mapping in nsJISx4051LineBreaker to handle new classes in ICU58/Unicode 9, and add assertions to detect any future additions that will require further updates. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f3c054b6a4b976359cbb741604aa57d812a9cd
Bug 1315986 followup - The static_assert to check Gecko vs ICU versions is only relevant when ENABLE_INTL_API is true. Bustage fix for Android on a CLOSED TREE.
Group: core-security → layout-core-security
https://hg.mozilla.org/mozilla-central/rev/c17531742f63
https://hg.mozilla.org/mozilla-central/rev/15f3c054b6a4

thanks Jonathan!
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tracking 52- since this is now fixed.
Group: layout-core-security → core-security-release
Group: core-security-release
Version: unspecified → 52 Branch
You need to log in before you can comment on or make changes to this bug.