283089 - Buffer overrun in extensions/transformiix/source/xslt/txXSLTNumberCounters.cpp

Status: RESOLVED WORKSFORME

(Core :: XSLT, defect)

Description

[David Gardiner]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Running PREfast static code analysis gives the following warning:

txxsltnumbercounters.cpp(200) : warning 201: Buffer overrun for stack buffer
'buf': index 11 exceeds maximum valid index 10.
problem occurs in function 'txAlphaCounter::appendNumber'

If you create an array [11] i think that means the last element is [10].

-dave

Reproducible: Always

Steps to Reproduce:

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll take this

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

No need to nullterminate, we pass a length to append

Comment 3

[Axel Hecht]

I wouldn't like this codepath with aNumber==0. That'd call
aDest.Append(buf + 11, 0)
which isn't catching the 0 length before accessing the char*.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

First off this code should never be called with aNumber < 1. Also, I just looked
briefly, but it seemed like .Append was even able to handle a null-data being
passed, or does it have specialcased code for that?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I stepped through the code and it didn't seem to access the data if length was 0

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 175085 [details] [diff] [review]
patch to fix

Although I find it a bit worrying too that we'd be able to call Append with
|buf + 11|.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: another patch

You're all wossies ;)

Ok, to this one keeps an extra unused position at the end of the buffer in case
we append an empty buffer or .Append accesses data past the end of the buffer.

I reduced the size of the buffer since an 'alpha-number' is maximum 7 chars
even for unsigned 32bit numbers.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 175289 [details] [diff] [review]
another patch

Actually, I'd be fine with buf[7] :-/.

Comment 9

[Axel Hecht]

this will have a merge conflict from bug 287290, which just contains wallpaper.
I still think we should land this patch as clean up sugar.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ugh, dbaron beat me to it. I'll check this in once the tree reopens though.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Solving this as worksforme since dbaron checked in a similar patch before this one got done. If someone else wants to land this patch feel free to.