Closed Bug 126941 Opened 21 years ago Closed 20 years ago

nsFormSubmission::EncodeVal return value not being checked for nsnull

Categories

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

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.1alpha

People

(Reporter: john, Assigned: john)

References

Details

(Whiteboard: [FIX])

Attachments

(1 file)

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().
Will fix ASAP after 117442 is fixed.
Depends on: 117442
Target Milestone: --- → mozilla1.1
Status: NEW → ASSIGNED
Jkeiser: Update: 117442 was marked dupe of 115906 which is marked Verified Fixed
Attached patch PatchSplinter Review
This does the trick.
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
Attachment #91195 - Flags: review+
make that: "fix that and r=sicking" ;-)
Whiteboard: [FIX]
@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 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
Attachment #91195 - Flags: superreview+
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 on attachment 91195 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #91195 - Flags: approval+
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...
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
verifying by reporter's/fixer's coments since I have no way to verify this.
Status: RESOLVED → VERIFIED
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.