16688 - nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect, P3)

Description

[kipp]

I found these gems today in the nsFontMetricsGTK.cpp file:

gint
nsFontMetricsGTK::GetWidth(nsFontGTK* aFont, const PRUnichar* aString,
  PRUint32 aLength)
{
  XChar2b buf[512];
  gint len = aFont->mCharSetInfo->Convert(aFont->mCharSetInfo, aString, aLength,
    (char*) buf, sizeof(buf));
  return gdk_text_width(aFont->mFont, (char*) buf, len);
}

void
nsFontMetricsGTK::DrawString(nsDrawingSurfaceGTK* aSurface, nsFontGTK* aFont,
  nscoord aX, nscoord aY, const PRUnichar* aString, PRUint32 aLength)
{
  XChar2b buf[512];
  gint len = aFont->mCharSetInfo->Convert(aFont->mCharSetInfo, aString, aLength,
    (char*) buf, sizeof(buf));
  ::gdk_draw_text(aSurface->GetDrawable(), aFont->mFont, aSurface->GetGC(), aX,
    aY + aFont->mBaselineAdjust, (char*) buf, len);
}

Comment 1

[Erik van der Poel]

We are passing sizeof(buf) to Convert(), which checks that value. So there
should be no problem.

Or are you saying that it is common to have strings longer than 512 chars
presented in one call?

Comment 2

[kipp]

You need a loop in both of those routines because you can be presented with very
large words (e.g. binary data or pre formatted data).

Comment 3

[Frank Tang]

I think it is very possible to have size > 512 pass in for at least GetWidth. 
Your code should do dynamic memory allocation when (and only when) that happen.

Comment 4

[Erik van der Poel]

I don't think this is important. I am planning to propose a new architecture
for GetWidth anyway, which will involve converting characters to glyphs, and
measuring one glyph at a time, until we reach the end of the current horizontal
space, and then return from the method. In the meantime, I don't think the
512 buffer limit will bite us.

Comment 5

[Erik van der Poel]

Moving some of my M14s to M15. Please add comments if you disagree.

Comment 6

[leger]

Bulk move of all "Output" component bugs to new "DOM to Test Conversion" 
component.  Output will be deleted as a component.

Comment 7

[Erik van der Poel]

Prioritizing my bugs. This one is now M20.

Comment 8

[Frank Tang]

reassign to bstell
bstell, please talk to erik and figure out this is important or not. Mark it as 
M22 for now.
Change the summary to 
buffer overrun likely's in nsFontMetricsGTK::GetWidth/DrawString

Comment 9

[rbs]

*** Bug 67938 has been marked as a duplicate of this bug. ***

Comment 10

[Boris Zbarsky [:bzbarsky]]

If bug 67938 is indeed a dup of this, then this bug makes view source
essentially unusable for any page that decides it does not need line breaks in
the source (a lot of them).  Keyword fun...

Comment 11

[kill this account]

Attachment: patch to make GetWidth/DrawString loop to handle long input strings (does not contain fixes for MathML)

Comment 12

[kill this account]

can I get a review of this?

thanks

Comment 13

[kill this account]

attachment http://bugzilla.mozilla.org/showattachment.cgi?attach_id=34177
changes:

 nsFontGTK::Convert() to pass in the address of the string length so I can 
   determine how much was consumed

 GetWidth() and DrawString() loop over the input if necessary

This does not fix MathML as it was not clear how to sum up the metrics.

Comment 14

[Frank Tang]

r=ftang

Comment 15

[rbs]

> This does not fix MathML as it was not clear how to sum up the metrics.
There is an overloaded operator += that will do the job for you. The main 
difference is that instead of starting with 'gint outLen = 0', you just need to 
initialize with the metrics of the first fragement.

I can give my r=, provided you fix mathml too. If you need more documentation,
see nsIRenderingContext.h. The variants of GetWidth/DrawString that do font 
switching loop too.

As for your patch, 
- you are modifying the const aString parameter with apparently no good reasons 
to do so.
- the prefix 'a' is for arguments. Local variables (const PRUnichar* aStringEnd) 
need not use that prefix.

Comment 16

[kill this account]

Attachment: revised patch: change outLen to width, fix to commented out code

Comment 17

[kill this account]

I did not see rbs' comments so please disregard attachment 34636 [details] [diff] [review].

I will change aStringEnd to stringEnd.

I'll take a look a the MathML part.

rbs: you said:
> you are modifying the const aString parameter with apparently no 
> good reasons to do so.

Are you refering to this:

  aString += PR_MAX(srcLen,1);

I believe this is necessary to walk along the string.

Comment 18

[rbs]

Yes, but it is better to use a temporary variable since the caller is expecting 
aString to be kept as is. (Someone changing the code at a higher-level may be 
bitten later.)

Comment 19

[kill this account]

Isn't the "PRUnichar* aString" a pass-by-value?

Comment 20

[kill this account]

Attachment: revised patch with MathML added

Comment 21

[kill this account]

I've added and compiled the MathML code and I believe it is correct but I 
really don't know how to test it.

Could someone test it? It would help if during the test the buffer size
was reduced from 512 to say 52 bytes.

Comment 22

[rbs]

Looks like you attached the same patch as before.

>Isn't the "PRUnichar* aString" a pass-by-value?

What I meant was to use PRUnichar* string = aString, (some casting may be neeed) 
so that even the pointer itself won't change.

Comment 23

[kill this account]

Attachment: Revised patch with MathML added (I swear)

Comment 24

[kill this account]

Isn't it a pass-by-value of the pointer?

It is true that sometime in the future one might add code to this routine that
wanted the original value. Generally in small routines I try not to add
code bloat trying to forsee future contingencies.

If you really want to leave aString undistrubed I will make a copy of
aString.

Comment 25

[rbs]

Only one minor point and it is ready (I can live with your patch as
it stands). For:

+    aBoundingMetrics += bm;

you need something like:

PRBool firstTime = PR_TRUE; // outside the loop

...

if (firstTime) {
  firstTime = PR_FALSE;
  aBoundingMetrics  = bm; // initialization
}
else {
  aBoundingMetrics += bm;
}

[This is because the leftbearing and rightbearing work in different ways, and
other values can be negative (instead of zero) at initialization]

Comment 26

[kill this account]

Attachment: patch with MathML initialization addition

Comment 27

[rbs]

OK, great. rr=rbs  (rr = real review :-) - The other folks on other platforms
could follow suit if they want things like view-source to work great.

Comment 28

[kill this account]

changed the title from:

  buffer overrun likely's in nsFontMetricsGTK::GetWidth/DrawString

to:

  nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars

Comment 29

[rbs]

Something to fix at your check-in time ('||' instead of '&&')
+  if (!aString && 0 >= aLength)
+    return NS_OK;
 
-  if (aString && 0 < aLength) {

=>
+  if (!aString || 0 >= aLength)

Comment 30

[Christopher Blizzard (:blizzard)]

+  gdk_error_trap_push(); // tell gdk not to exit if this fails
   GdkFont* gdkFont = ::gdk_font_load(mName);
+  gdk_error_trap_pop();

Why are those error traps there?

OK, I have to ask.  Why are we using a statically allocated buffer and using
loops intead of using some kind of dynamically allocated buffer?  If you're
worried about doing an allocation on every call you can use a global buffer that
is ever-increasing if you want.  Is there some X limitation that keeps a buffer
that is > 512 bytes working?

Also, I suspect that since we are doing small chunks that we are causing more
server round trips since much of this is calculated on the server.

Comment 31

[kill this account]

oops: this is a fix for bug 59355

+  gdk_error_trap_push(); // tell gdk not to exit if this fails
   GdkFont* gdkFont = ::gdk_font_load(mName);
+  gdk_error_trap_pop();

I'll take it out of this checkin and put it there.

Regarding dynamic vs static buffers:
We have been working for a long time with a fixed bugger and without looping.
I doubt that this code will loop except for view source (which is the reason
we never fixed this before this. I would not want the typical case to suffer
to support the infrequent case. Thus I'd prefer not to allocate memory.

Comment 32

[Christopher Blizzard (:blizzard)]

Then only allocate when you go over the 512 byte size.  That in itself would be
a lot less code and a lot less prone to errors than what you have there since it
would only require one check to see if it was bigger, one extra variable and a
check to see if you need to free it on the way out of the function.

Comment 33

[kill this account]

I only intended to spend a couple of hours on this old low priority bug.

I'm over that by a factor of 5 now

Marking future.

Comment 34

[kill this account]

Attachment: yataap (yet another try at a patch)

Comment 35

[kill this account]

grumble, grumble, grumble

Comment 36

[Roland Mainz]

Xlib-toolkit and Xprint have the same problem and should be patched, too...

Comment 37

[rbs]

This buffer is for notes you don't want to save, and for Lisp evaluation.
If you want to create a file, visit that file with C-x C-f,
then enter the text in that file's own buffer.

"your best reviewer may sometimes sounds like your worst enemy" :-) don't
remember the page where I saw that incisive bugzilla quotation that helps to
take things easy...

+         printf("alloced a buffer, %s %d\n", __FILE__, __LINE__);
a left over from your debugging

BufferAllocIfNeeded
BufferFree  -> may be should: BufferFreeIfNeeded, for symmetry?

The new code looks much elegant and easier to maintain - so after all, blizzard
saw right here :-)

r=rbs

Comment 38

[rbs]

"This buffer ... etc" -- emacs !!!

Comment 39

[Christopher Blizzard (:blizzard)]

Cool!  Just one comment.

You use the value of *oBufLen + 1 in order to allocate the size of the buffer
that you will write to in BufferAllocIfNeeded().  That's the return value of
|nsIUnicodeEncoder::GetMaxLength|.  Are you absolutely and completely sure that
GetMaxLength returns a value in bytes required and not the number of segments
that are needed?  My concern is that you are only allocating x * sizeof(char)
instead of x * sizeof(XChar2b).  If you are doing that then your buffer is half
the size it needs to be and you will be stomping all over memory like the jolly
green giant.  The header file documentation is a little sparse on what the
return value from that function is.

-  PRUnichar buf[512];
+  PRUnichar buf[5120];

Is that increase in size required for something?  A bigger buffer might be
better here, I'm just wondering.

And the printf just has to go! :)

OK.  Make me feel better about those and you will have an sr=blizzard

Comment 40

[kill this account]

Yes, the review process helped alot (grumble, grumble).

blizzard: thanks for pushing 

Unless someone minds I plan to move the buffer alloc/free routines to
the converter lib where it will be available to the whole app.

yaptf (yet another patch to follow) ...

Comment 41

[kill this account]

Attachment: patch with the alloc/free as a macro in the convert header

Comment 42

[rbs]

I am a bit suspicious about this sizeof(char*) -- it might actually returns the
size of the pointer -- what about passing the current length of the static
buffer to your macro too?

 nsFontGTKSubstitute::GetWidth(const PRUnichar* aString, PRUint32 aLength)
 {
-  PRUnichar buf[512];
+  PRUnichar buf[5120];  <--- you mean 512 here, right?

Apart from these two, my r=rbs still applies.

Comment 43

[kill this account]

> I am a bit suspicious about this sizeof(char*) -- it might actually 
> returns the size of the pointer -- what about passing the current 
> length of the static buffer to your macro too?

The code does work correctly, I looked at numbers in the debugger.
(For debug I made the buffer 10 chars which caused it to malloc
most but not all of the time.)

I presume you are speaking of:

 +//  sb = static buffer        (char *)

I'll change the comment to

 +//  sb = static buffer        (char[])

> -  PRUnichar buf[512];
> +  PRUnichar buf[5120];  <--- you mean 512 here, right?

The nsFontGTKSubstitute font code does not use a converter so I was not
able to fix it. As such I bumped out the buffer size to make it "real
unlikely" we would see this problem in the substitute code.

If you want I can take this out.

Comment 44

[rbs]

> I looked at numbers in the debugger.
This appears too sensitive to my taste, especially that it is sitting in a
nsI... I think passing the length will make things more robust and will make you
and me, and the next person feel better now, and in the months to come :-)
Without having to waste time re-checking again in the debugger when hunting for
something wrong...

For nsFontGTKSubstitute, since the actual size is the length of the input, what
about the vanilla Alloc/Free pair.

Comment 45

[kill this account]

> For nsFontGTKSubstitute, since the actual size is the length of the input

I thought substitute did transliteration (in which case the length is very
hard to figure without actually translating).

Comment 46

[kill this account]

Attachment: patch with sizeof(buf) as an explicit parameter

Comment 47

[Christopher Blizzard (:blizzard)]

Wait.  It's really easy to fix that buffer size problem in DrawString():

gint
nsFontGTKSubstitute::DrawString(nsRenderingContextGTK* aContext,
                                nsDrawingSurfaceGTK* aSurface,
                                nscoord aX, nscoord aY,
                                const PRUnichar* aString, PRUint32 aLength)
{
  PRUnichar buf[512];
  PRUnichar *bigBuf = nsnull;
  if (aLength > 512)
    bigBuf = nsMemory::Alloc(sizeof(PRUnichar) * aLength);
    if (!bigBuf)
      return NS_ERROR_OUT_OF_MEMORY;
  }
  PRUint32 len;
  if (bigBuf)
    len = Convert(aString, aLength, bigBuf, aLength);
  else
    len = Convert(aString, aLength, buf, sizeof(buf)/2);
  return mSubstituteFont->DrawString(aContext, aSurface, aX, aY,
                                     (bigBuf || buf), len);
  if (bigBuf)
    nsMemory::Free(bigBuf);
}

Please fix that.

Comment 48

[kill this account]

Chris, I will add that code but just for reference:

(from an email:)

> Brian Stell wrote: 
>   Frank, 
>   For substitution don't we do transliteration?
> Yes, we do 
>   If we do transliteration, can we predict the output size?
> No, we cannot. 

Comment 49

[kill this account]

Attachment: patch with mallocs for nsFontGTKSubstitute calls

Comment 50

[kill this account]

Can we get this approved fairly soon or should we mark it WONTFIX 
since these bits are already rotting ?

It took much longer than I ever expected and I cannot justify more
work on this.

Comment 51

[Christopher Blizzard (:blizzard)]

I didn't know that you were waiting for review.  You should shop around if you
haven't gotten a response or bug us more.

Of course, we're going to fix it.  Marking it wontfix just because you are
frustrated isn't an answer. :)

Comment 52

[Christopher Blizzard (:blizzard)]

r/sr=blizzard

Comment 53

[kill this account]

Chris: as always, thanks for you care and interest.
This bug is waiting on approval from drivers/PDT not r/sr.

The core issue is that in the past developers worked on bugs,
got reviewed, checked in.

Today till moz 1.0 all changes also need a drivers/PDT approval.
Hence, work done without this may be useless.

The patch is already bit rotting. By moz 1.0 it will need to be
rewritten. This is a fairly low priority bug. I should never have
put so many hours into it. Unless its priority changes I recommend we
not to spend any more time on it hence the WONTFIX. If you would 
prefer FUTURE that would be okay but I feel the WONTFIX is more 
representive of what will actually happen.

To avoid this wasteage of engineering time I recommend that between now
and moz 1.0 all bugs get pre-approved by PDT/drivers before any work
is done.

Comment 54

[Frank Tang]

The important question we need to answer before consider any work from now till 
moz0.9.2 branch off is 
What will happen to the user if we don't fix this bug?

Comment 55

[Roland Mainz]

ftang:
Simple answer: User will experience a crash sometimes...

Comment 56

[kill this account]

it should not crash but view page info will truncate long text lines.

Comment 57

[Roland Mainz]

asa:
Requesting a=asa/drivers

Comment 58

[Roland Mainz]

Per asa's comment:
Can someone here please email [e-mail] an a= request with the
following details:
- clear description of the problem
- description of the solution
- risk associated with the patch
- how well is the patch tested ?

Comment 59

[kill this account]

checked in 

Comment 60

[kill this account]

Attachment: patch; remove the debugging MOZ_MATHML define

Comment 61

[Frank Tang]

r=ftang of removing the first line of nsFontMetricsGTK.cpp

Comment 62

[sujay]

bstell, can you verify this bug and mark verified-fixed? thanks.

Comment 63

[kill this account]

this has been in a long time now without a problem