base64 encoder/decoder should use fallible allocation

RESOLVED FIXED in Firefox 39

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: Titi_Alone, Mentored)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Base64EncodeInputStream is used for base64-encoding blobs, canvas and is
used by nsScriptableBase64Encoder:
http://mxr.mozilla.org/mozilla-central/source/dom/workers/FileReaderSync.cpp#235
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#503
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsScriptableBase64Encoder.cpp#20

I think such uses should use fallible allocation, so we should change
the SetLength calls in Base64.cpp to use the fallible version:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/Base64.cpp#183
It already has some error checking for OOM there so it seems that
fallible allocation was the original intention.

Also, I think these should actually return the result from the call:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsScriptableBase64Encoder.cpp#20

Comment 1

3 years ago
See https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation for an explanation of how to use fallible memory allocation.
Mentor: josh@joshmatthews.net
Whiteboard: [lang=c++]
Summary: bas64 encoder/decoder should use fallible allocation → base64 encoder/decoder should use fallible allocation
(Assignee)

Comment 2

3 years ago
Can I please work on this bug?
(Assignee)

Comment 3

3 years ago
Created attachment 8567506 [details] [diff] [review]
Patch for bug 1133063
Attachment #8567506 - Flags: superreview?
Attachment #8567506 - Flags: review?(josh)

Updated

3 years ago
Assignee: nobody → titou125
Comment on attachment 8567506 [details] [diff] [review]
Patch for bug 1133063

No need for a superreview.  Those are not used much any more.
Attachment #8567506 - Flags: superreview?

Comment 5

3 years ago
Comment on attachment 8567506 [details] [diff] [review]
Patch for bug 1133063

Review of attachment 8567506 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! Let's get the review of Nathan, who owns this code (see https://wiki.mozilla.org/Modules/Core).
Attachment #8567506 - Flags: review?(nfroyd)
Attachment #8567506 - Flags: review?(josh)
Attachment #8567506 - Flags: feedback+
Comment on attachment 8567506 [details] [diff] [review]
Patch for bug 1133063

Review of attachment 8567506 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with one minor change below.  Please upload a new patch with the change and flag me for review.

::: xpcom/io/Base64.cpp
@@ +184,1 @@
>    if (aDest.Length() != count + aOffset) {

Can you please move the SetLength call into the if's condition, like:

  if (!aDest.SetLength(count + aOffset, mozilla::fallible)) {
    ...
  }
Attachment #8567506 - Flags: review?(nfroyd) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8570000 [details] [diff] [review]
New patch for bug 1133063
Attachment #8567506 - Attachment is obsolete: true
Attachment #8570000 - Flags: review?(nfroyd)
Comment on attachment 8570000 [details] [diff] [review]
New patch for bug 1133063

Review of attachment 8570000 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8570000 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/44495ed6c618
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.