Closed Bug 287112 Opened 19 years ago Closed 17 years ago

atob() on a string with a '\n' throws OOM exception

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: caillon, Assigned: jst)

References

()

Details

(Keywords: helpwanted)

Attachments

(2 files, 1 obsolete file)

Not sure what the correct behavior is, but it should not claim to be OOM.
nsGlobalWindow::Atob
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#4019

codetovalue
http://lxr.mozilla.org/seamonkey/source/nsprpub/lib/libc/src/base64.c#172

codetovalue, passed \n, will result in -1 returned to decode4to3 (next function
in base64.c, line 216).

Because of this, decode4to3 assumes failure, returns it, and everything
generally unravels.

>	plc4.dll!codetovalue(unsigned char c='
')  Line 176	C
 	plc4.dll!decode4to3(const unsigned char * src=0x04157790, unsigned char *
dest=0x041577c8)  Line 216 + 0xe	C
 	plc4.dll!decode(const unsigned char * src=0x04157790, unsigned int
srclen=0x00000006, unsigned char * dest=0x041577c8)  Line 323 + 0xd	C
 	plc4.dll!PL_Base64Decode(const char * src=0x04157790, unsigned int
srclen=0x00000006, char * dest=0x041577c8)  Line 416 + 0x11	C
 	gklayout.dll!nsGlobalWindow::Atob(const nsAString & aAsciiBase64String={...},
nsAString & aBinaryData={...})  Line 4016 + 0x15	C++
Keywords: helpwanted
So the basic problem is that PL_Base64Decode does:

  A null is retuned if the allocation fails, or if the source is not well-coded.

So from the caller, all we know is that one of those two happened.  Given that,
perhaps NS_ERROR_FAILURE is a more appropriate error to throw?
Actually, atob() fails, period:
javascript:try{alert(atob("foo"));}catch(e){alert(e)}
From: http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#4084

4084 nsGlobalWindow::Atob(const nsAString& aAsciiBase64String,
4085                      nsAString& aBinaryData)
4086 {
4087   aBinaryData.Truncate();
4088 
4089   if (!Is8bit(aAsciiBase64String)) {
4090     return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
4091   }
4092 
4093   char *base64 = ToNewCString(aAsciiBase64String);
4094   if (!base64) {
4095     return NS_ERROR_OUT_OF_MEMORY;
4096   }
4097 

Here, ToNewCString returns false on a plaintext string ?

Actually, there are two definitions of ToNewCString() in 
The second one skips the sting conversion.

See lines 316  and 355 in :
http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsReadableUtils.cpp



4098   PRInt32 dataLen = aAsciiBase64String.Length();
4099 
4100   PRInt32 resultLen = dataLen;
4101   if (base64[dataLen - 1] == '=') {
4102     if (base64[dataLen - 2] == '=') {
4103       resultLen = dataLen - 2;
4104     } else {
4105       resultLen = dataLen - 1;
4106     }
4107   }
4108 
4109   resultLen = ((resultLen * 3) / 4);
4110 
4111   char *bin_data = PL_Base64Decode(base64, aAsciiBase64String.Length(),
4112                                    nsnull);
4113   if (!bin_data) {
4114     nsMemory::Free(base64);
4115 
4116     return NS_ERROR_OUT_OF_MEMORY;
4117   }
4118 
4119   CopyASCIItoUCS2(Substring(bin_data, bin_data + resultLen), aBinaryData);
4120 
4121   nsMemory::Free(base64);
4122   PR_Free(bin_data);
4123 
4124   return NS_OK;
4125 }
Hellekin: The problem is that PL_Base64Decode returns null both for OOM and if
the input string is not valid base64.
(In reply to comment #3)
> Actually, atob() fails, period:
> javascript:try{alert(atob("foo"));}catch(e){alert(e)}

atob('foo') doesn't throw an error on my build (Gecko/20051006 Firefox/1.4.1).

But I've found lots of other strings that throw the OOM error. Any 1-length
string throws the error. Any string containing a character that's not an
alphanumerical character or '+' or '/' throw the error. So yeah, the passed
string needs to be in base64 (which of course makes it useless).
atob is not "useless" - it was a misunderstanding on my part.

Email correspondence w/ Christian Biesinger:
> > > ? Why does it make that useless? The entire point of atob is to convert 
> > > base64 to the original data...
> > 
> > I was under the impression that btoa was the one that converts base64 to 
> > the original data. I've never used atob and btoa tho (except when 
> > testing this bug), so am I mistaken?
> 
> Yes - the "b" stands for "binary", the "a" for ASCII. Therefore, the "a" 
> actually means "base64". Yeah, this is somewhat confusing...

This really needs to be documented on devmo. Google search on atob and btoa was
fruitless...
I think this should be fixed. Getting an out of memory error for a bad base64 string is very confusing. When I saw that I had to dig through LXR to understand what was really going on.
Attached patch Wrong diff. (obsolete) — Splinter Review
Assignee: general → jst
Status: NEW → ASSIGNED
Attachment #252123 - Flags: superreview?(jonas)
Attachment #252123 - Flags: review?(jonas)
Attached patch Fix.Splinter Review
Attachment #252123 - Attachment is obsolete: true
Attachment #252124 - Flags: superreview?(jonas)
Attachment #252124 - Flags: review?(jonas)
Attachment #252123 - Flags: superreview?(jonas)
Attachment #252123 - Flags: review?(jonas)
Attachment #252123 - Attachment description: Fix. → Wrong diff.
Comment on attachment 252124 [details] [diff] [review]
Fix.

I take it that PL_Base64Decode never goes over resultLen characters.

Given that's ensured, r/sr=sicking
Attachment #252124 - Flags: superreview?(jonas)
Attachment #252124 - Flags: superreview+
Attachment #252124 - Flags: review?(jonas)
Attachment #252124 - Flags: review+
Attached patch Fix that landed.Splinter Review
Fixed on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: