329612 - nsTextFrame.cpp::PaintAsciiText may use uninitialized variable

Status: RESOLVED INVALID

(Firefox :: General, defect)

Description

[georgi - hopefully not receiving bugspam]

nsTextFrame.cpp::PaintAsciiText may use uninitialized variable

1) http://lxr.mozilla.org/seamonkey/source/layout/generic/nsTextFrame.cpp#3834
const char* text;
at this point |text| may point to anything depending on previous stack content.

2) http://lxr.mozilla.org/seamonkey/source/layout/generic/nsTextFrame.cpp#3888
if this branch is hit, |text| is uninitialized and may point to anything.

later in this method |text| is used at least for reading, but if it is used
for writing this may be exploitable.

so if 2) is hit, at least a crash may be guaranteed

Comment 1

[georgi - hopefully not receiving bugspam]

1) http://lxr.mozilla.org/seamonkey/source/layout/generic/nsTextFrame.cpp#3834
const char* text;
making the above
const char* text=0;

will give deterministic behavior imho (most oses don't start mapping from 0).

building on linux with gcc -Wuninitialized gives 133 warnings - some seem harmless, but some like this are not clear.

since uninitialized variables are exploitable, probably the warnings should be shut.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Except that |text| is only used if |textLength > 0|, which will won't be the case in the only case where |text| is uninitialized, right?

Comment 3

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> Except that |text| is only used if |textLength > 0|, which will won't be the
> case in the only case where |text| is uninitialized, right?
> 

yes, it seems like so. i have missed that condition, so this bug is invalid.

though some of the rest of uninitiliazed gcc warnings are more complex and depend on inter method conditions, so it is very difficult for me to tell value reachability. 

Comment 4

[georgi - hopefully not receiving bugspam]

http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsISVGPathFlatten.h
86   float Length() {
 87     float xx, yy, length = 0;
 88     for (PRUint32 i = 0; i < count; i++) {
 89       if (type[i] == NS_SVGPATHFLATTEN_LINE) {
 90         float dx = x[i] - xx;
 91         float dy = y[i] - yy;
 92         length += sqrt(dx*dx + dy*dy);
 93       }
 94       xx = x[i];
 95       yy = y[i];
 96     }
 97     return length;
 98   }
 99 };

in this case isn't very clear if |type[0] == NS_SVGPATHFLATTEN_LINE| may be true

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

This bug is morphing, but the SVG guys might want to take a look at comment 4.

Comment 6

[georgi - hopefully not receiving bugspam]

(In reply to comment #4)
> http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsISVGPathFlatten.h
> 86   float Length() {
>  87     float xx, yy, length = 0;
>  88     for (PRUint32 i = 0; i < count; i++) {
>  89       if (type[i] == NS_SVGPATHFLATTEN_LINE) {
>  90         float dx = x[i] - xx;
>  91         float dy = y[i] - yy;
>  92         length += sqrt(dx*dx + dy*dy);
>  93       }
>  94       xx = x[i];
>  95       yy = y[i];
>  96     }
>  97     return length;
>  98   }
>  99 };
> 
> in this case isn't very clear if |type[0] == NS_SVGPATHFLATTEN_LINE| may be
> true
> 

this seems fixed by Bug 333631

Comment 7

[Jonathan Watt [:jwatt]]

Indeed. Sorry I missed comment 4.

Closing as invalid since the original issue was a false positive.