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

Status: RESOLVED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Not sure what the correct behavior is, but it should not claim to be OOM.

Comment 1

[Alex Vincent [:WeirdAl]]

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++

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Hellekin]

Actually, atob() fails, period:
javascript:try{alert(atob("foo"));}catch(e){alert(e)}

Comment 4

[Hellekin]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

Hellekin: The problem is that PL_Base64Decode returns null both for OOM and if
the input string is not valid base64.

Comment 6

[Yuh-Ruey Chen]

(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

[Yuh-Ruey Chen]

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

[Wladimir Palant]

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.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Wrong diff.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Fix.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 12

[Johnny Stenback (:jst)]

Attachment: Fix that landed.

Comment 13

[Johnny Stenback (:jst)]

Fixed on the trunk.