534171 - nsBulletFrame::AppendCounterText can return an uninitialized value

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: fix: initialize success to PR_TRUE

GCC 4.3.3 on Linux gives us this warning, when building mozilla-central:
> /layout/generic/nsBulletFrame.cpp: In static member function ‘static PRBool nsBulletFrame::AppendCounterText(PRInt32, PRInt32, nsString&)’:
> /layout/generic/nsBulletFrame.cpp:983: warning: ‘success’ may be used uninitialized in this function

Link to code:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBulletFrame.cpp#979

The method "AppendCounterText" is basically just a giant switch statement, after which we return the value of |success|.  The problem is that |success| gets declared without an initial value, and the first four case clauses don't set it to anything. (while the other cases all set it to the return value of some helper function.)

So if we hit any of those first four clauses, I think AppendCounterText will return whatever random uninitialized value it ends up finding in |success|.

It seems like it'd be simplest & cleanest to just initialize |success| as PR_TRUE when we declare it.  We'd initially assume that we'll succeed, since failure is impossible in the simple cases, and then if we try to do something complicated that may fail (i.e. call a helper function), we'll update that assumption by modifying |success|.

(It looks like this is exactly what we did in an older version of this function, "GetListItemText", before bug 3247's patch landed -- but we removed the initialization in that patch for some reason.)

Comment 1

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

Comment on attachment 417080 [details] [diff] [review]
fix: initialize success to PR_TRUE

r=dbaron

Comment 2

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/a781ccdadb08