nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars

VERIFIED FIXED in Future

Status

()

Core
Serializers
P3
normal
VERIFIED FIXED
19 years ago
17 years ago

People

(Reporter: kipp, Assigned: kill this account)

Tracking

({dataloss, pp})

Trunk
Future
x86
Linux
dataloss, pp
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

(Reporter)

Description

19 years ago
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

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

Comment 2

19 years ago
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).

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M14

Comment 3

19 years ago
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

19 years ago
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

19 years ago
Moving some of my M14s to M15. Please add comments if you disagree.
Target Milestone: M14 → M15

Comment 6

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

Updated

19 years ago
Component: Output → DOM to Text Conversion

Comment 7

18 years ago
Prioritizing my bugs. This one is now M20.
Target Milestone: M15 → M20

Comment 8

18 years ago
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
Assignee: erik → bstell
Status: ASSIGNED → NEW
Summary: buffer overrun likely's → buffer overrun likely's in nsFontMetricsGTK::GetWidth/DrawString
Target Milestone: M20 → M22
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

18 years ago
Target Milestone: --- → mozilla0.9.1

Comment 9

17 years ago
*** Bug 67938 has been marked as a duplicate of this bug. ***
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...
Keywords: correctness, mozilla0.9.1, pp
(Assignee)

Comment 11

17 years ago
Created attachment 34177 [details] [diff] [review]
patch to make GetWidth/DrawString loop to handle long input strings (does not contain fixes for MathML)
(Assignee)

Comment 12

17 years ago
can I get a review of this?

thanks
(Assignee)

Comment 13

17 years ago
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.

Updated

17 years ago
Whiteboard: patch need review 2001-05-11 20:37

Comment 14

17 years ago
r=ftang

Comment 15

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

Comment 16

17 years ago
Created attachment 34636 [details] [diff] [review]
revised patch: change outLen to width, fix to commented out code
(Assignee)

Comment 17

17 years ago
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

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

Comment 19

17 years ago
Isn't the "PRUnichar* aString" a pass-by-value?
(Assignee)

Comment 20

17 years ago
Created attachment 34660 [details] [diff] [review]
revised patch with MathML added
(Assignee)

Comment 21

17 years ago
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

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

Comment 23

17 years ago
Created attachment 34673 [details] [diff] [review]
Revised patch with MathML added (I swear)
(Assignee)

Comment 24

17 years ago
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

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

Comment 26

17 years ago
Created attachment 34697 [details] [diff] [review]
patch with MathML initialization addition

Comment 27

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

Comment 28

17 years ago
changed the title from:

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

to:

  nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars
Summary: buffer overrun likely's in nsFontMetricsGTK::GetWidth/DrawString → nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars
(Assignee)

Updated

17 years ago
Whiteboard: patch need review 2001-05-11 20:37 → patch waiting for super-review 2001-05-15 21:30

Comment 29

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

Comment 31

17 years ago
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.

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

Comment 33

17 years ago
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.
Target Milestone: mozilla0.9.1 → Future
(Assignee)

Comment 34

17 years ago
Created attachment 34908 [details] [diff] [review]
yataap (yet another try at a patch)
(Assignee)

Comment 35

17 years ago
grumble, grumble, grumble

Comment 36

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

Comment 37

17 years ago
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

17 years ago
"This buffer ... etc" -- emacs !!!
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
(Assignee)

Comment 40

17 years ago
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) ...
Whiteboard: patch waiting for super-review 2001-05-15 21:30
(Assignee)

Comment 41

17 years ago
Created attachment 35103 [details] [diff] [review]
patch with the alloc/free as a macro in the convert header

Comment 42

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

Comment 43

17 years ago
> 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

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

Comment 45

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

Comment 46

17 years ago
Created attachment 35158 [details] [diff] [review]
patch with sizeof(buf) as an explicit parameter
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.
(Assignee)

Comment 48

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

Comment 49

17 years ago
Created attachment 36616 [details] [diff] [review]
patch with mallocs for nsFontGTKSubstitute calls
(Assignee)

Comment 50

17 years ago
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.
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. :)
r/sr=blizzard
(Assignee)

Comment 53

17 years ago
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

17 years ago
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

17 years ago
ftang:
Simple answer: User will experience a crash sometimes...
(Assignee)

Comment 56

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

Comment 57

17 years ago
asa:
Requesting a=asa/drivers

Comment 58

17 years ago
Per asa's comment:
Can someone here please email drivers@mozilla.org 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 ?
Keywords: dataloss
(Assignee)

Comment 59

17 years ago
checked in 
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 60

17 years ago
Created attachment 40663 [details] [diff] [review]
patch; remove the debugging MOZ_MATHML define

Comment 61

17 years ago
r=ftang of removing the first line of nsFontMetricsGTK.cpp

Comment 62

17 years ago
bstell, can you verify this bug and mark verified-fixed? thanks.
(Assignee)

Comment 63

17 years ago
this has been in a long time now without a problem
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.