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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: caillon, Assigned: jst)
References
()
Details
(Keywords: helpwanted)
Attachments
(2 files, 1 obsolete file)
1.58 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
Details | Diff | Splinter Review |
Not sure what the correct behavior is, but it should not claim to be OOM.
Comment 1•19 years ago
|
||
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++
Updated•19 years ago
|
Keywords: helpwanted
Comment 2•19 years ago
|
||
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 }
Comment 5•19 years ago
|
||
Hellekin: The problem is that PL_Base64Decode returns null both for OOM and if the input string is not valid base64.
Comment 6•19 years ago
|
||
(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).
Comment 7•19 years ago
|
||
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...
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 9•17 years ago
|
||
Assignee: general → jst
Status: NEW → ASSIGNED
Attachment #252123 -
Flags: superreview?(jonas)
Attachment #252123 -
Flags: review?(jonas)
Assignee | ||
Comment 10•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
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+
Assignee | ||
Comment 12•17 years ago
|
||
Assignee | ||
Comment 13•17 years ago
|
||
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.
Description
•