126941 - nsFormSubmission::EncodeVal return value not being checked for nsnull

Status: VERIFIED FIXED

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

Description

[John Keiser (jkeiser)]

EncodeVal() is not having its return value checked for nsnull, which can cause
crashes in low-memory situations.  One instance is checking for nsnull, but then
doing ToNewCString().

Comment 1

[John Keiser (jkeiser)]

Will fix ASAP after 117442 is fixed.

Comment 2

[Vladimir Ermakov]

Jkeiser: Update: 117442 was marked dupe of 115906 which is marked Verified Fixed

Comment 3

[John Keiser (jkeiser)]

Attachment: Patch

This does the trick.

Comment 4

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

Comment on attachment 91195 [details] [diff] [review]
Patch

>@@ -258,8 +258,9 @@
>    *
>    * @param aStr the string to encode
>    * @param aEncoded the encoded string [OUT]
>+   * @return NS_ERROR_OUT_OF_MEMORY if we run out of memory

nit: just say @return errorcode or some suce, that way we can add more errors
if needed and people won't do stupid things like |if (rv ==
NS_ERROR_OUT_OF_MEMORY)|

>-  delete processedValue;
>-

This seems to be missing from the ProcessAndEncode function.

r=sicking

Comment 5

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

make that: "fix that and r=sicking" ;-)

Comment 6

[John Keiser (jkeiser)]

@return is intended for multiple values.  When you return error codes, you
should list them all like this:

@return NS_ERROR_A
@return NS_ERROR_B

This allows people to anticipate why the code might fail.  It's like @throws in
Java.  It is debatable whether NS_ERROR_OUT_OF_MEMORY needs to appear in these
statements, but I don't think it hurts.

Excellent catch on the delete.  I meant to move it but never did.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 91195 [details] [diff] [review]
Patch

>+   * @return NS_ERROR_OUT_OF_MEMORY if we run out of memory

Shouldn't this be @throws/@exception?  In an IDL file it would definitely be; I
think we should do the same thing here.

>+  /**
>+   * Roll up the data so far and add it to the multiplexed data stream.
>+   */

"the data we have so far"

>+   * @return NS_ERROR_OUT_OF_MEMORY if out of memory

Again, @exception

With that and the |delete| thing, sr=bzbarsky

Comment 8

[John Keiser (jkeiser)]

I'm not sure if @throws works, esp. since it's a return value.  That, and I have
seen MS COM documentation that uses @return for this.  I'll look it up and see
if @throws actually works, but I'm not sure if we should.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 91195 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.1

Comment 10

[Boris Zbarsky [:bzbarsky]]

Let me put it this way.  In IDL if I have:

foo func(bar);

it would have an @return, an @param, and @throws.

The C++ equivalent of that is

NS_IMETHOD func(bar, foo*);

And I feel that this should also have an @return, an @param, and @throws since
it is functionally the same.  If it were not an IMETHOD it would have a real
return value, but here the return is just our internal version of throwing an
exception...

Comment 11

[John Keiser (jkeiser)]

Fix checked in.

Comment 12

[Vladimir Ermakov]

verifying by reporter's/fixer's coments since I have no way to verify this.