1232100 - Check charsWritten in non-debug builds.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wei Wu [:wuwei UTC+8]]

JS_vsnprintf() only checks `charsWritten` in debug build. In release build, if the `charsWritten` become 0, we might overwrite a byte which should not be touched. Although this issue is not sec-related (guess), adding a extra check might be good for consistency.

Comment 1

[Wei Wu [:wuwei UTC+8]]

Attachment: patch

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Wei Wu [:w :wuwei UTC+8] from comment #0)
> if the `charsWritten` become 0

I don't *think* that this case can happen although it's pretty murky.  

dosprintf() will always try to write at least one byte (the null terminator), and we already checked the length of our output buffer was not zero above.  dosprintf() is fallible (and we don't check return value ), however I couldn't find a path where it fails that isn't because it hit the buffer limit.

It probably warrants a release assert or at least a comment though :)

Comment 3

[Wei Wu [:wuwei UTC+8]]

(In reply to Jon Coppeard (:jonco) from comment #2)
> (In reply to Wei Wu [:w :wuwei UTC+8] from comment #0)
> > if the `charsWritten` become 0
> 
> I don't *think* that this case can happen although it's pretty murky.  
> 
> dosprintf() will always try to write at least one byte (the null
> terminator), and we already checked the length of our output buffer was not
> zero above.  dosprintf() is fallible (and we don't check return value ),
> however I couldn't find a path where it fails that isn't because it hit the
> buffer limit.
> 
> It probably warrants a release assert or at least a comment though :)

oh sorry i didn't know the MOZ_RELEASE_ALERT macro.
I actually don't know in what scenario charsWritten could be zero or not, just noticed
that we have a MOZ_ASSERT here, and we do had a "non-zero check" like
|if(charsWritten > 0 && ss.cur[-1] != '\0')| before (see attachment 8687998 [details] [diff] [review]).

Comment 4

[Wei Wu [:wuwei UTC+8]]

Comment on attachment 8697762 [details] [diff] [review]
patch

Hi luke,

as discussed above, this patch might not be ready for review, so I'd cancel the review for now.
Sorry for my disturbance.

Comment 5

[Wei Wu [:wuwei UTC+8]]

Attachment: use MOZ_RELEASE_ASSERT

Replace MOZ_ASSERT with MOZ_RELEASE_ASSERT.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a4f006fa68e

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a4f006fa68e