195635 - valgrind doesn't like Xprint (invalid reads in XpuParseMediumSourceSize)

Status: RESOLVED FIXED

(Core Graveyard :: Printing: Xprint, defect)

Description

[Andrew Schultz]

If I run Mozilla under valgrind and try to do any printing (just bring up the
print dialog), valgrind complains a lot.  There actually appear to be two
problems.  The first is that for this line:

num_input_items = sscanf(d, "%s %f %f %f %f",...);

valgrind reports that sscanf is reading beyond the bounds of |d|.  Printing it
with printf, it sometimes looks ok, but often appears to be not null-terminated:
"false 6.3500 209.5500 6.3500 273.0500}ܱqB"
"false 6.3500 721.6500 6.3500
1023.6500}MONETARY=en_US;LC_MESSAGES=en_US;LC_PAPER=en_US;LC_NAME=en_US;Y`"

the second problem is that valgrind reports that when the locale is set back to
its original value, that the string had already been freed:

  cur_locale = setlocale(LC_NUMERIC, NULL);
  setlocale(LC_NUMERIC, "C");  <== this frees cur_locale
  num_input_items = sscanf(...);
  setlocale(LC_NUMERIC, cur_locale);

strcpy'ing ought to fix the second issue.

Comment 1

[Masaki Katakai]

Assigned to Roland.

Comment 2

[Roland Mainz]

Attachment: Patch for 2003-03-11-08-trunk to fix the 2nd issue

The patch fixes the (more serious) 2nd issue of the bug. The first one is
little bit harder to fix (it looks like |XpuEnumerateXpAttributeValue| screams
for a re-write - however the only clean fix for it would be to import the xrm
sources from the X.org tree (which is likely impossible with mozilla.org's
current policy about importing non-mozilla libraries (or even small parts of
them) into the mozilla/-tree... ;-( ))

Comment 3

[Roland Mainz]

Attachment: Better patch for 2003-03-11-08-trunk to catch both issues

Slightly better patch, fixing the 2nd issue and (maybe) the 1st one, too...

Comment 4

[Roland Mainz]

Comment on attachment 117437 [details] [diff] [review]
Better patch for 2003-03-11-08-trunk to catch both issues

Requesting r= ...

Comment 5

[Andrew Schultz]

Comment on attachment 117437 [details] [diff] [review]
Better patch for 2003-03-11-08-trunk to catch both issues

couldn't you just null-terminate |name| after the value=>name copy loop?

662	s++;
663   }
664   while(*s);
+++   *d = '\0';
665	
666   /* seperate medium name from string */
667   d = (char *)search_next_space(name);

sscanf should null-terminate the |boolbuf| half of |name| for you.

Comment 6

[Roland Mainz]

Andrew Schultz wrote:
> (From update of attachment 117437 [details] [diff] [review])
> couldn't you just null-terminate |name| after the value=>name copy loop?

Does that fix "valgrind"'s complaints ?

Comment 7

[Andrew Schultz]

> Does that fix "valgrind"'s complaints ?

yes, valgrind liked patch1 + comment 5.
I was initially confused by the triplicity of |name|.  |medium_name| and
|boolbuf| were already being null-terminated.  it was only the |d| part that was
unterminated, and that can/should be done after the copy loop.

isn't the if statement at the top of the function mostly backwards?

-  if( value && value[0]!='{' && value[0]!='\0' )
+  if ( !value || value[0]!='{' )
     return(False);

I was trying to consider what sort of edge cases might cause trouble for |name|

Comment 8

[Roland Mainz]

Attachment: New patch for 2003-03-11-08-trunk

Comment 9

[Roland Mainz]

Andrew Schultz wrote:
> isn't the if statement at the top of the function mostly backwards?
>
> -  if( value && value[0]!='{' && value[0]!='\0' )
> +  if ( !value || value[0]!='{' )
>     return(False);

Good question. The change looks OK, however I prefer to not change that now
because I would have to run the whole testsuite against that change to ensure
that I don't produce a new problem if something goes wrong.

Comment 10

[Roland Mainz]

Comment on attachment 117679 [details] [diff] [review]
New patch for 2003-03-11-08-trunk

Requesting r= ...

Comment 11

[Andrew Schultz]

> The change looks OK, however I prefer to not change that now
> because I would have to run the whole testsuite against that change to ensure
> that I don't produce a new problem if something goes wrong.

well, for instance, passing in a NULL |value| will *always* crash (at
strlen(value)) and the "if" lets it through explicitly (so returning on a NULL
|value| can't break anything).

Passing a valid pointer to NULL (which is also explicitly allowed) would do very
bad things (possibly crash) because the while(*s) will be looking at the 2nd
element, whose value could be random.  The while loop would continue reading
random memory and writing into unallocated space (value_len=0, sizeof(name)=4)
until it gets lucky and hits a NULL byte.

Comment 12

[Andrew Schultz]

Comment on attachment 117679 [details] [diff] [review]
New patch for 2003-03-11-08-trunk

r=ajschult

Comment 13

[Roland Mainz]

Comment on attachment 117679 [details] [diff] [review]
New patch for 2003-03-11-08-trunk

Requesting sr= ...

Comment 14

[Roland Mainz]

Patch checked-in by timeless
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1048129200&maxdate=1048129380&who=timeless%25mozdev.org),
marking bug as FIXED.