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

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cbook, Assigned: jfkthame)

Tracking

(Blocks: 1 bug, {regression})

52 Branch
mozilla52
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52- fixed)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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!
(Reporter)

Updated

2 years ago
Flags: needinfo?(jfkthame)
(Assignee)

Comment 1

2 years ago
(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)
(Reporter)

Comment 2

2 years ago
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)
(Reporter)

Comment 3

2 years ago
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
(Assignee)

Comment 4

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

Comment 5

2 years ago
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
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox50: --- → unaffected
status-firefox-esr45: --- → unaffected
(Reporter)

Comment 6

2 years ago
[Tracking Requested - why for this release]:
regression - bughunter found
tracking-firefox52: --- → ?
(Assignee)

Comment 7

2 years ago
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)

Comment 8

2 years ago
Created 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
Attachment #8808999 - Flags: review?(masayuki)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 10

2 years ago
(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.)
(Assignee)

Comment 11

2 years ago
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
(Assignee)

Comment 12

2 years ago
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
(Reporter)

Comment 13

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

thanks Jonathan!
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tracking 52- since this is now fixed.
tracking-firefox52: ? → -
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.