Closed Bug 1232100 Opened 4 years ago Closed 4 years ago

Check charsWritten in non-debug builds.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: wuwei, Assigned: wuwei)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8697762 - Flags: review?(luke)
(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 :)
(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 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.
Attachment #8697762 - Flags: review?(luke)
Replace MOZ_ASSERT with MOZ_RELEASE_ASSERT.
Attachment #8698968 - Flags: review?(jcoppeard)
Attachment #8697762 - Attachment is obsolete: true
Attachment #8698968 - Flags: review?(jcoppeard) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5a4f006fa68e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.