273025 - bad logic results in potential leak xor crash based on flow

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[timeless]

the url contains code flow analysis. if you're interested in fixing this bug,
other developers on irc will gladly help.

there are two bugs here.
1. if {headers#118} and {!NS_SUCCEEDED(rv)#125} then buf#120 leaks.
2. if {!headers#118} and {NS_SUCCEEDED(rv)#125} then AdoptData#126 is an api
violation and should eventually lead to a crash

Comment 1

[Boris Zbarsky [:bzbarsky]]

So... I tried to sort through this, but the plug-in code is completely schizoid about who owns what data.  None of it is documented, and I got lost in a maze of twisty functions, all with similar names....  :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

Specifically, what is the API contract for ownership of the POST data strings going across NPN_PostURL and such?

Comment 3

[Michael Wu [:mwu]]

Attachment: Possible fix, v1

This code is.. interesting. One of the callers of NewPluginURLStream (which ends up calling NS_NewPluginPostDataStream) frees the string buffer it passes to NewPluginURLStream based on if isFile is true. This is no good since an error can cause the string to get leaked. Need to investigate other callers and possibly some plugins code to figure out if this fix is the right idea.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Almost definitely need to look at some plugins' code: they'd be calling NPN_PostURL.

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 274245 [details] [diff] [review]
Possible fix, v1

Alright, I've investigated a bit further and I'm pretty sure there's no problem with AdoptData - in all cases, it's allocated correctly by us. However, there are quite a number of ways to leak the post data if errors occur.

Comment 6

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v1, (1/2)

This patch is just code cleanups. The behavior of the code should not change at all. This just made it easier for me to read the code and put together the patch that actually fixes this bug.

Comment 7

[Johnny Stenback (:jst)]

Michael, could you attach a diff -w here to make reviewing a bit easier?

Comment 8

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v1, (2/2)

And this patch actually fixes the bug, by refactoring much of the code. This should fix a large number of potential memory leaks when using PostURL and make it much more clear who owns what.

It fixes:
1. A potential leak of dataToPost (in PostURL) if the function it's passed to does not free the string on error. (either NewPluginURLStream or GetURL - and they almost never free the string on an error) This is achieved by giving the string data to a nsIStringInputStream ASAP and passing that input stream around instead. This also eliminates the NS_NewPluginPostDataStream function.
2. A potential leak of listenerPeer in the error path by using a nsRefPtr.
3. A leak of the tmpfile string returned by CreateTmpFileToPost. A nsIFile is now returned instead, which is more efficient and leak resistant.

Comment 9

[Michael Wu [:mwu]]

(In reply to comment #8)
> Created an attachment (id=275014) [details]
> Improve GetURL/PostURL code, v1, (2/2)
> 
Oh, and one last bug this fixes:
4. A potential failure to delete the temporary file due to errors. The temporary file is now opened as soon as possible to ensure it gets deleted at some point.

Comment 10

[Michael Wu [:mwu]]

(In reply to comment #7)
> Michael, could you attach a diff -w here to make reviewing a bit easier?
> 
Most of it wasn't just whitespace changes, so it doesn't seem to change much with -w.

Comment 11

[timeless]

Comment on attachment 275014 [details] [diff] [review]
Improve GetURL/PostURL code, v1, (2/2)

nsPluginInstanceOwner::GetURL - line too long
createTmpFileToPost - please spell out Temp since we're already changing the api
nsIFile **pTmpFile - args should be aFoo
you can do NS_ADDREF(*aTempFile = tempFile)

thanks for working on this.

i don't suppose you would be interested in making test plugins :).

Comment 12

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v2, (1/2)

Just some unbitrotting.

Comment 13

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v2, (2/2)

Address timeless' comments.

Comment 14

[Mike Schroepfer]

Ping - is this bug still live? Asking because it is on the leaks list...

Comment 15

[Johnny Stenback (:jst)]

Given how fragile this code is I don't think we should bother with this for 1.9 unless someone can show that this is a source for a non-trivial amount of leaks. We've fixed leaks around this code before and caused plugins to break, which led us to undo the leak fix...

Comment 16

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

That sounds ok to me. It's something we should keep in mind once we start doing trace-malloc testing and plugins.

Comment 17

[timeless]

this is only a leak if you're already out of memory (which happens often for embedders but rarely in most other environments).

unfortunately my group is being bugged by people who intentionally run our device out of memory and then complain about leaks and bad behavior.

so technically i care.

jst: could you please review this? it isn't fare to the assignee that you haven't and it isn't fair to anyone else. it's also a bad example to people who might want to contribute showing how little we apparently care about contributions.

i'm fine with not committing this (for 1.9), but i think you at least owe michael the review (and any of us the chance to use the reviewed patch in our modified variants instead of encouraging people to randomly include unreviewed patches).

Comment 18

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v3, (1/2)

Updated to mozilla-central. This patch does less than before due to cleanups in nsPluginHost.

Comment 19

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v3, (1/2), no indentation changes

Same thing without all the indentation changes that confuse diff.

Comment 20

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v3, (2/2)

Updated to mozilla-central. Note that this bug doesn't just leak when you're out of memory - it leaks even when you have memory and no errors occur. (#3 in https://bugzilla.mozilla.org/show_bug.cgi?id=273025#c8 )

Comment 21

[Michael Wu [:mwu]]

Holding off the review request for the second patch until I convince myself I remember what the code was trying to do before.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 402003 [details] [diff] [review]
Improve GetURL/PostURL code, v3, (2/2)

r+sr=jst for this cleanup (indentation changes should of course be included when checking in). Now's a pretty good time to do the commit the rest of this fix as well.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 402003 [details] [diff] [review]
Improve GetURL/PostURL code, v3, (2/2)

Er, wrong attachment.

Comment 24

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v4, (1/2) [Checkin: Comment 26]

Applies cleanly against mozilla-central. Gonna mark checkin-needed since I have no idea how commits work in this post-cvs world.

Comment 25

[Michael Wu [:mwu]]

Comment on attachment 402003 [details] [diff] [review]
Improve GetURL/PostURL code, v3, (2/2)

Ok I figured out what I did and it looks fine. Mochitests pass too, though the tests (bug 517078) don't cover the PostURL file=true codepath.

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 408225 [details] [diff] [review]
Improve GetURL/PostURL code, v4, (1/2)
[Checkin: Comment 26]


http://hg.mozilla.org/mozilla-central/rev/fa0a3e9b1f54

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 402003 [details] [diff] [review]
Improve GetURL/PostURL code, v3, (2/2)

- In nsPluginInstanceOwner::GetURL():

+    if (aHeadersDataLen <= 0)

aHeadersDataLen is unsigned, so use !aHeadersDataLen or aHeadersDataLen == 0 here.

+    char *buf = (char *)nsMemory::Alloc(aHeadersDataLen);
+    if (!buf)
+      return NS_ERROR_OUT_OF_MEMORY;
+    memcpy(buf, aHeadersData, aHeadersDataLen);
+
+    nsCOMPtr<nsIStringInputStream> sis = do_CreateInstance("@mozilla.org/io/string-input-stream;1");
+    if (!sis) {
+      NS_Free(buf);
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+
+    sis->AdoptData(buf, aHeadersDataLen);

All of the above could be replaced with a call to sis->SetData((char *)aHEadersData, aHeadersDataLen) no?

- In nsPluginHost::PostURL():

     // we use nsIStringInputStream::adoptDataa()
     // in NS_NewPluginPostDataStream to set the stream
     // all new data alloced in  ParsePostBufferToFixHeaders()
     // well be nsMemory::Free()d on destroy the stream

Update this comment, NS_NewPluginPostDataStream() no longer exists after this patch.

- In nsPluginHost::CreateTempFileToPost():

+      NS_ADDREF(*aTmpFile = tempFile);

Maybe do *aTmpFile = tempFile.forget().get() here instead to avoid extra refcounting?

r=jst with that looked into.

Comment 28

[Michael Wu [:mwu]]

Attachment: Improve GetURL/PostURL code, v4 (2/2)

All review comments addressed. Thanks jst!

Comment 29

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/03362a8f3b65