366902 - [Pango backend] Random crashes/abort when page contains null characters in justified sections

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mike Hommey [:glandium]]

This has been reported as Debian bug #406698.

I'm going to attach a stripped down test case that leads to a freeze on my computer.

When there are nul characters in justified blocks of texts (with spaces), the browser freezes or crashes in a random fashion.

Running through valgrind reveals the following problem as a probable source of the problem:
==8089== Invalid write of size 4
==8089==    at 0x77E4598: nsFontMetricsPango::DrawStringSlowly(char const*, unsigned short const*, unsigned, _GdkDrawable*, _GdkGC*, int, int, _PangoLayoutLine*, int const*) (nsFontMetricsPango.cpp:1338)
==8089==    by 0x77E76A5: nsFontMetricsPango::DrawString(unsigned short const*, unsigned, int, int, int, int const*, nsRenderingContextGTK*, nsDrawingSurfaceGTK*) (nsFontMetricsPango.cpp:788)
==8089==    by 0x77D9CF9: nsRenderingContextGTK::DrawString(unsigned short const*, unsigned, int, int, int, int const*) (nsRenderingContextGTK.cpp:1324)
==8089==    by 0x5F2ACCD: nsTextFrame::RenderString(nsIRenderingContext&, nsStyleContext*, nsPresContext*, nsTextFrame::TextPaintStyle&, unsigned short*, int, int, int, int, int, SelectionDetails*) (nsTextFrame.cpp:3083)
==8089==    by 0x5F2D4B4: nsTextFrame::PaintTextSlowly(nsPresContext*, nsIRenderingContext&, nsStyleContext*, nsTextFrame::TextPaintStyle&, int, int) (nsTextFrame.cpp:3364)
==8089==    by 0x5F2F6A2: nsTextFrame::Paint(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, unsigned) (nsTextFrame.cpp:1604)
==8089==    by 0x5EDF368: nsContainerFrame::PaintChild(nsPresContext*, nsIRenderingContext&, nsRect const&, nsIFrame*, nsFramePaintLayer, unsigned) (nsContainerFrame.cpp:282)
==8089==    by 0x5ECC5C6: nsBlockFrame::PaintChild(nsPresContext*, nsIRenderingContext&, nsRect const&, nsIFrame*, nsFramePaintLayer, unsigned) (nsBlockFrame.h:286)
==8089==    by 0x5ED1137: nsBlockFrame::PaintChildren(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, unsigned) (nsBlockFrame.cpp:6470)
==8089==    by 0x5EF727D: nsHTMLContainerFrame::PaintDecorationsAndChildren(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, int, unsigned) (nsHTMLContainerFrame.cpp:136)
==8089==    by 0x5ED0CD6: nsBlockFrame::Paint(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, unsigned) (nsBlockFrame.cpp:6364)
==8089==    by 0x5EDF368: nsContainerFrame::PaintChild(nsPresContext*, nsIRenderingContext&, nsRect const&, nsIFrame*, nsFramePaintLayer, unsigned) (nsContainerFrame.cpp:282)
==8089==  Address 0x967EFEC is 0 bytes after a block of size 44 alloc'd
==8089==    at 0x401D7C1: operator new[](unsigned) (vg_replace_malloc.c:195)
==8089==    by 0x77E4545: nsFontMetricsPango::DrawStringSlowly(char const*, unsigned short const*, unsigned, _GdkDrawable*, _GdkGC*, int, int, _PangoLayoutLine*, int const*) (nsFontMetricsPango.cpp:1329)
==8089==    by 0x77E76A5: nsFontMetricsPango::DrawString(unsigned short const*, unsigned, int, int, int, int const*, nsRenderingContextGTK*, nsDrawingSurfaceGTK*) (nsFontMetricsPango.cpp:788)
==8089==    by 0x77D9CF9: nsRenderingContextGTK::DrawString(unsigned short const*, unsigned, int, int, int, int const*) (nsRenderingContextGTK.cpp:1324)
==8089==    by 0x5F2ACCD: nsTextFrame::RenderString(nsIRenderingContext&, nsStyleContext*, nsPresContext*, nsTextFrame::TextPaintStyle&, unsigned short*, int, int, int, int, int, SelectionDetails*) (nsTextFrame.cpp:3083)
==8089==    by 0x5F2D4B4: nsTextFrame::PaintTextSlowly(nsPresContext*, nsIRenderingContext&, nsStyleContext*, nsTextFrame::TextPaintStyle&, int, int) (nsTextFrame.cpp:3364)
==8089==    by 0x5F2F6A2: nsTextFrame::Paint(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, unsigned) (nsTextFrame.cpp:1604)
==8089==    by 0x5EDF368: nsContainerFrame::PaintChild(nsPresContext*, nsIRenderingContext&, nsRect const&, nsIFrame*, nsFramePaintLayer, unsigned) (nsContainerFrame.cpp:282)
==8089==    by 0x5ECC5C6: nsBlockFrame::PaintChild(nsPresContext*, nsIRenderingContext&, nsRect const&, nsIFrame*, nsFramePaintLayer, unsigned) (nsBlockFrame.h:286)
==8089==    by 0x5ED1137: nsBlockFrame::PaintChildren(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, unsigned) (nsBlockFrame.cpp:6470)
==8089==    by 0x5EF727D: nsHTMLContainerFrame::PaintDecorationsAndChildren(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, int, unsigned) (nsHTMLContainerFrame.cpp:136)
==8089==    by 0x5ED0CD6: nsBlockFrame::Paint(nsPresContext*, nsIRenderingContext&, nsRect const&, nsFramePaintLayer, unsigned) (nsBlockFrame.cpp:6364)

Tracing with gdb helped understanding what was going on.

the aText argument of nsFontMetricsPango::DrawStringSlowly is generated, in nsFontMetricsPango::DrawString, from aString with g_utf16_to_utf8, but since aString contains a null character, g_utf16_to_utf8 ends before reaching the end of the string. In this case, the initialization of utf8_spacing in nsFontMetricsPango::DrawStringSlowly is done using the strlen of aText, which is then much smaller than aLength, and may lead to problems with the rest of the code.

This is reproducible with 1.8.0 and 1.8.1 branches.

Note that if you remove the space between 3 and ., it doesn't freeze anymore, but "Freeze !" is not displayed.

I'm working on a patch on initialization of utf8_spacing, which should fix the freeze, but probably not the display issue, for which a wrapper around g_utf16_to_utf8 may be needed. Or maybe removing the null characters from the input would be even better, I don't know...

I don't know if this may be exploited either, but I'm afraid it may...

Comment 1

[Mike Hommey [:glandium]]

Attachment: testcase

Comment 2

[Mike Hommey [:glandium]]

(In reply to comment #0)
> I don't know if this may be exploited either, but I'm afraid it may...

Actually, thinking more about it, I think it may not...

Comment 3

[Mike Hommey [:glandium]]

Attachment: Fix for the crash/freeze

This fix is tested and works, but as stated, doesn't solve the fact that everything between a null character and the end of a line disappears, which may be subject of another bug, though fixing this other bug may make the fix here useless.

Comment 4

[Mike Hommey [:glandium]]

As far as I can tell, the null character is not valid either in SGML or in XML. Maybe we could replace it with the replacement character (U+FFFD, question mark in a diamond)

Comment 5

[Mike Hommey [:glandium]]

Also note that putting a NULL character entity (�) in the file instead of the NULL character itself has the same effect.

Comment 6

[Mike Hommey [:glandium]]

Ah, the XFT backend puts a "0000 in a square" glyph...

Comment 7

[Mike Hommey [:glandium]]

Attachment: Alternative patch

This patch implements a wrapper around g_utf16_to_utf8 in nsFontMetricsPango.cpp to replace NULL characters with the replacement character (U+FFDF). Since it removes the NULL characters, it makes my previous patch useless.
Please tell me if this is the right approach and/or if it would be preferable to fix this another way.

Comment 8

[Mike Hommey [:glandium]]

Seems the patch is not enough to have NULL character replaced when text is not justified...

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 251382 [details] [diff] [review]
Alternative patch

Removing review request. This patch doesn't do the right thing. It'd like to hear comments on the issue, though.

Comment 10

[Mike Hommey [:glandium]]

Maybe nsTextFrame::PrepareUnicodeText could be a good place to do this...

Comment 11

[Mike Hommey [:glandium]]

Attachment: Yet another way to fix the issue

Another way to fix this issue is to discard NULL characters, instead of replacing them. That will remove these characters with all backends.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that this bug should fix on gfx.

Because the null character should be rendered by missing glyph. The patch breaks the behavior in other platforms too.

# see also bug 362712 for trunk.

Comment 13

[Mike Hommey [:glandium]]

(In reply to comment #12)
> I think that this bug should fix on gfx.
> 
> Because the null character should be rendered by missing glyph. The patch
> breaks the behavior in other platforms too.

Why not replace it with U+FFDF for all platforms (which can be done in nsTextTransform too), then ? 

Comment 14

[Simon Montagu :smontagu]

(In reply to comment #13)
> Why not replace it with U+FFDF for all platforms (which can be done in
> nsTextTransform too), then ? 

I assumed before that "FFDF" was just a typo, but since you seem to be using it consistently let me point out that it should be "FFFD"

Comment 15

[Mike Hommey [:glandium]]

It's actually twice the same typo, damn fingers.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #13)
> (In reply to comment #12)
> > I think that this bug should fix on gfx.
> > 
> > Because the null character should be rendered by missing glyph. The patch
> > breaks the behavior in other platforms too.
> 
> Why not replace it with U+FFDF for all platforms (which can be done in
> nsTextTransform too), then ? 

The current reviewing patch doesn't replace, it removes the null characters.

Comment 17

[Mike Hommey [:glandium]]

> The current reviewing patch doesn't replace, it removes the null characters.

You were proposing to do it at gfx level, I'm just asking if it would be fine to have it at nsTextTransform level, considering it would impact all platforms. 

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #17)
> > The current reviewing patch doesn't replace, it removes the null characters.
> 
> You were proposing to do it at gfx level, I'm just asking if it would be fine
> to have it at nsTextTransform level, considering it would impact all platforms. 

On other platforms, there are no problem. I think that the behavior should not be changed for compatibility for old versions, especially, on branch.

Comment 19

[Mike Hommey [:glandium]]

I think it would be better if all platforms did the same thing. The XFT backend puts a 0000 in a square glyph ; that is obviously not possible with pango, so another replacement character may have to be used, or another solution be found. I doubt windows and macosX versions do the same... BTW, what do they do ?

Also, the null character is not supposed to be present at all in the documents and is actually not valid sgml or xml. I don't see why it can be so important to keep compatibility with old versions.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #19)
> I doubt windows and macosX versions do the same... BTW, what do they do?

Even if they have same behavior in current version, we cannot say that it is true in future versions.

And bug 362712 cannot be reproduced on latest pango version. I think that the crash is a bug of pango, if so, we should not change the behavior in XP level only for a bug for a lib of a platform. I cannot believe that is correct logic.

Comment 21

[Mike Hommey [:glandium]]

The crash is not a bug of pango, it is a bug in nsFontMetricsPango::DrawStringSlowly, that is addressed by my first patch.

The other issue is that null characters split content and make half of it (what is after the null character) not be displayed. This is not a bug in pango either. It is only due to the fact that pango uses utf-8 input and stops at null characters that are usually used to end strings.

FWIW, 1.5.0.9 version of Firefox on windows does display... nothing. It just either discards the character or display a 0-widthed empty character. Dropping the null character like what does my last patch would not change how pages are displayed on windows at least.

I'll repeat myself, but NULL characters not being standard, it would be be a good thing if they were treated equally on all platforms.

Comment 22

[Mike Hommey [:glandium]]

(In reply to comment #21)
> I'll repeat myself, but NULL characters not being standard, it would be be a
> good thing if they were treated equally on all platforms.

Which is not the case at the moment, considering this bug or not.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #21)
> The crash is not a bug of pango, it is a bug in
> nsFontMetricsPango::DrawStringSlowly, that is addressed by my first patch.

I don't think so, because |nsFontMetricsPango::DrawString| sends the string length to |pango_layout_set_text|, so the pango should care the null character as a character.

I think that roc is a best people for this bug.

Comment 24

[Mike Hommey [:glandium]]

(In reply to comment #23)
> I don't think so, because |nsFontMetricsPango::DrawString| sends the string
> length to |pango_layout_set_text|, so the pango should care the null character
> as a character.

This is how pango works, it stops at null characters. It's in the API description.

Comment 25

[Mike Hommey [:glandium]]

Moreover nsFontMetricsPango::DrawString transforms the utf-16 in utf-8 with g_utf16_to_utf8, which also stops at null character.

And nsFontMetricsPango::DrawStringSlowly uses strlen of this utf-8 string to initialize utf8storage. strlen also stops at null characters.

The latter is the reason of the crash.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #24)
> (In reply to comment #23)
> > I don't think so, because |nsFontMetricsPango::DrawString| sends the string
> > length to |pango_layout_set_text|, so the pango should care the null character
> > as a character.
> 
> This is how pango works, it stops at null characters. It's in the API
> description.

You're right. I confirmed it in the API reference. But I still think that this should be fixed on gfx level.

Comment 27

[Mike Hommey [:glandium]]

I did some more tests

Safari doesn't display null characters
IE6 on windows doesn't display null characters
Firefox 2.0.0.1 on macosX doesn't display null characters
Lynx doesn't display null characters
Konqueror replaces null characters by spaces

So, to summarize, so far, only mozilla xft backend displays something impacting the display (a 0000 in a square impacts much more than a space).

Are you really sure you don't want to just discard them everywhere ?

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #27)
> Are you really sure you don't want to just discard them everywhere ?

Yes. See the HTML spec:
http://www.w3.org/TR/html401/charset.html#h-5.4

> 1. Adopt a clearly visible, but unobtrusive mechanism to alert the user of missing resources.

I think that we should display the null character.

Comment 29

[Mike Hommey [:glandium]]

(In reply to comment #28)
> I think that we should display the null character.

Then it should be displayed on all platforms, which is what I proposed in comment #13. 

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #29)
> (In reply to comment #28)
> > I think that we should display the null character.
> 
> Then it should be displayed on all platforms, which is what I proposed in
> comment #13. 

What glyph should be used should be determined by platform rules. I think that the code should be in gfx, not layout.

Comment 31

[Mike Hommey [:glandium]]

Note that 5.4 in HTML spec is about undisplayable characters because of lack of
font or internal technical issues. It presuposes the HTML file is valid.

Here is what the w3c validator has to say about a null character:
Error  Line 13 column 9: non SGML character number 0.

1.2.3 ...Freeze !

You have used an illegal character in your text. HTML uses the standard UNICODE
Consortium character repertoire, and it leaves undefined (among others) 65
character codes (0 to 31 inclusive and 127 to 159 inclusive) that are sometimes
used for typographical quote marks and similar in proprietary character sets.
The validator has found one of these undefined characters in your document. The
character may appear on your browser as a curly quote, or a trademark symbol,
or some other fancy glyph; on a different computer, however, it will likely
appear as a completely different character, or nothing at all.

Your best bet is to replace the character with the nearest equivalent ASCII
character, or to use an appropriate character entity. For more information on
Character Encoding on the web, see Alan Flavell's excellent HTML Character Set
Issues reference.

This error can also be triggered by formatting characters embedded in documents
by some word processors. If you use a word processor to edit your HTML
documents, be sure to use the "Save as ASCII" or similar command to save the
document without formatting information.

Here is what is has to say about null character entity:
Warning  Line 9 column 12: reference to non-SGML character.

3.2.1 ...�Freeze !

You've included a character reference to a character that is not defined in the
document type you've chosen. This is most commonly caused by numerical
references to characters from vendor proprietary character repertoires. Often
the culprit will be fancy or typographical quote marks from either the Windows
or Macintosh character repertoires.

The solution is to reference UNICODE characters instead. A list of common
characters from the Windows character repertoire and their UNICODE equivalents
can be found in the document "On the use of some MS Windows characters in HTML"
maintained by Jukka Korpela <[e-mail]>.

Comment 32

[Mike Hommey [:glandium]]

> What glyph should be used should be determined by platform rules. I think that
> the code should be in gfx, not layout.

Aren't invalid characters in input character set already replaced by a replacement character way before that ?

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I agree the null character is illegal character for web contents. Therefore, the illegal character should be displayed to users that includes the author of web contents. If nobody can know the illegal character(s) is in the content, that is not good thing.

But if the process makes damage to the performance, it may be good thing that the code (replacing to U+FFFD) is in layout.

Comment 34

[Mike Hommey [:glandium]]

Or maybe it could be done even earlier at the uconv level ? But then, what about characters such as  and such, that are also not authorized ?

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

That is very high risk. The way should not be accepted on branch.

Comment 36

[Simon Montagu :smontagu]

Filtering illegal characters at the uconv (or parser) level can be a security risk. I'm agnostic about whether layout or gfx is preferable, but we certainly shouldn't do it any earlier.

Comment 37

[Mike Hommey [:glandium]]

At layout level, what are the characters that were invalid in the input document like ? Are they replaced by U+FFFD or are they replaced by something dependent on the platform ?
I remember from when hacking the native uconv implementation that there is indeed code to deal with the substitution right after getting the string from the nsIUnicodeDecoder implementation (while the implementation itself is also able to deal, or supposed to, with the substitution), but I can't remember what character is used then, and can't find this code again but that may be tiredness...

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 251386 [details] [diff] [review]
Yet another way to fix the issue

I'm a little hesitant to make a change to cross-platform code in a security release to work around issues in an unsupported (?) gfx backend.  And it sounds like others don't like this either, although I'd be interested in roc's opinion.

That said, the patch looks fine to me.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we just make it #ifdefed on the use of the Pango backend? I'm hesitant about the assumptions that may have been made about the operation of IS_DISCARDABLE.

Comment 40

[Mike Hommey [:glandium]]

Actually, there is a bug with the Xft backend that is linked to this issue... If you copy text with a null character in the middle (that is shown as 0000 in a square), everything after the null character is lost when you paste the text.

Others seem to prefer having the null character replaced by something else (possibly U+FFFD), but I'd need to have an answer to comment #37.

Comment 41

[Simon Montagu :smontagu]

Illegal characters in the source encoding are replaced by U+FFFD on all platforms, either by the decoders themselves or by the parser (this inconsistency is a Bad Thing, but that's not relevant here).

Comment 42

[Simon Montagu :smontagu]

(In reply to comment #37)
> I remember from when hacking the native uconv implementation that there is
> indeed code to deal with the substitution right after getting the string from
> the nsIUnicodeDecoder implementation (while the implementation itself is also
> able to deal, or supposed to, with the substitution), but I can't remember what
> character is used then, and can't find this code again but that may be
> tiredness...
 
I expect you mean http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/parser/htmlparser/src/nsScanner.cpp&rev=3.148&mark=356-383#350

Comment 43

[Mike Hommey [:glandium]]

(In reply to comment #42)
> I expect you mean
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/parser/htmlparser/src/nsScanner.cpp&rev=3.148&mark=356-383#350

Ah yes, that's it. And there is similar code in js and css parsers IIRC.

Anyways, in the end, what is the fix that you would like to see ? Something in layout, but #ifdefed, maybe on xft and pango backends ? or in gfx ?

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's take your layout fix, #ifdef Pango.

Is the pasting bug in comment #40 actually fixed by modifying IS_DISCARDABLE? I'd be shocked if it was.

On trunk this should be fixed at the textrun level, so don't worry about trunk.

Comment 45

[Mike Hommey [:glandium]]

(In reply to comment #44)
> Let's take your layout fix, #ifdef Pango.

So in the end you don't care about having it replaced by U+FFFD ?

> Is the pasting bug in comment #40 actually fixed by modifying IS_DISCARDABLE?
> I'd be shocked if it was.

It doesn't.
 
> On trunk this should be fixed at the textrun level, so don't worry about trunk.

Where should it be fixed on 1.8 branch ?

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> So in the end you don't care about having it replaced by U+FFFD ?

No, I don't.

> Where should it be fixed on 1.8 branch ?

In IS_DISCARDABLE.

Actually on trunk we might want to fix it at that level too, but the code has changed a lot. I'll take responsibility for that!

Comment 47

[Mike Hommey [:glandium]]

I meant, for the copy/paste issue.

Are you okay if the IS_DISCARDABLE patch is #ifdefed for pango AND xft ?

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's have a separate bug for the copy/paste issue.

Let's just do this for Pango. If the xft behaviour is a problem we should treat it in a separate bug. I haven't heard that it's a big problem.

Comment 49

[Mike Hommey [:glandium]]

The rationale for doing it for both is to have all platforms displaying the same thing, i.e. nothing, instead of the 0000 in a square that xft displays

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I understand, but that probably isn't serious enough for a branch fix, given that we have (presumably) behaved this way with Xft for many releases. Possibly we could get it in under the relaxed rules for Linux branch landings, but it would still have to be serious.

On trunk, definitely we should converge on common behaviour and discard nulls on all platforms.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> On trunk, definitely we should converge on common behaviour and discard nulls
> on all platforms.

Roc, the null character is rendered by missing glyph on trunk of Win32. I think that we should not change this behavior, see comment 33. (The page authors should remove the null character from theirs contents. But if all browsers don't display the illegal characters, the authors cannot find the bug of the contents.)

Comment 52

[Mike Hommey [:glandium]]

Last patch in bug 357733 solves the NULL character handling problem at gfx level by not using g_utf16_to_utf8 and replacing by 0xff.

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 251386 [details] [diff] [review]
Yet another way to fix the issue

Redirecting review request to roc.

Is this being proposed for branch, trunk, or both?  I still think it seems scary for branch.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch is irrelevant for trunk, where nulls are handled at the textrun level. In comment #48 I mentioned that I'd be happy with this patch if it was branch only and Pango only.

Comment 55

[Reed Loden [:reed]]

Mike, any update to address roc's last comment (comment #54)?

Comment 56

[Mike Hommey [:glandium]]

That will require some testing with trunk which i'm not ready yet to do.

Comment 57

[Mike Hommey [:glandium]]

I can confirm this is fixed on trunk.