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

Status: RESOLVED FIXED

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

Description

[Carsten Book [:Tomcat]]

Attachment: 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!

Comment 1

[Jonathan Kew [: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...

Comment 2

[Carsten Book [:Tomcat]]

tested http://www.espn.com/nfl/scoreboard on a linux trunk asan build from taskcluster and seems that page hung the browser after awhile

Comment 3

[Carsten Book [:Tomcat]]

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

Comment 4

[Jonathan Kew [:jfkthame]]

I suspect this may be a regression from bug 1265631, though I haven't spotted what's wrong with the code there yet...

Comment 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:
regression - bughunter found

Comment 7

[Jonathan Kew [:jfkthame]]

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

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: 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

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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!

Comment 10

[Jonathan Kew [:jfkthame]]

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

Comment 11

[Jonathan Kew [:jfkthame]]

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

Comment 12

[Jonathan Kew [:jfkthame]]

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.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c17531742f63
https://hg.mozilla.org/mozilla-central/rev/15f3c054b6a4

thanks Jonathan!

Comment 14

[Marcia Knous [:marcia]]

Tracking 52- since this is now fixed.