[NaCl SDK] nacl_io: Fix leak when recv'ing from udp socket.

[NaCl SDK] nacl_io: Fix leak when recv'ing from udp socket.

Packet::Copy AddRefs the address resource returned from
PPB_UDPSocket::RecvFrom. But the convention is that when returning a new
resource, the resource will already have a refcount of 1. So the resource has
an leaked reference and will never be destroyed.

This memory leak cannot be detected from untrusted code (I overrode
mmap/mmunmap, but was unable to find it), but can be seen from the Chrome task
manager.

See user report here:
https://groups.google.com/d/msg/native-client-discuss/fEc6v7_wjkI/lnFNO09yw9kJ

BUG=none
R=noelallen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290691

[yzshen1, 2014-08-19 18:37:29]

https://codereview.chromium.org/487363003/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/socket/udp_node.cc (right):

https://codereview.chromium.org/487363003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/socket/udp_node.cc:137:
filesystem()->ppapi()->ReleaseResource(addr_);
drive-by:
Maybe we could consider "safer" ref-counting.
- addr_ is not reset but this class doesn't own a ref after this
ReleaseResource() call.
- if Start() is called but Run() isn't, then addr_ is eventually leaked.

(I don't know much about this part of code base, though. Just my 2 cents. :))

[noelallen1, 2014-08-19 20:46:59]

LGTM

In theory you could release it in the virtual destructor, but this has the added
benefit of correctness even if the object was reused somehow.

https://codereview.chromium.org/487363003/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/socket/udp_node.cc (right):

https://codereview.chromium.org/487363003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/socket/udp_node.cc:137:
filesystem()->ppapi()->ReleaseResource(addr_);
While it is possible to call Start without calling Run, it shouldn't be possible
RecvFrom to populate addr_ without also calling the callback correct?


On 2014/08/19 18:37:29, yzshen1 wrote:
> drive-by:
> Maybe we could consider "safer" ref-counting.
> - addr_ is not reset but this class doesn't own a ref after this
> ReleaseResource() call.
> - if Start() is called but Run() isn't, then addr_ is eventually leaked.
> 
> (I don't know much about this part of code base, though. Just my 2 cents. :))

[yzshen1, 2014-08-19 20:53:16]

(No need to wait for my LG.)

https://codereview.chromium.org/487363003/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/socket/udp_node.cc (right):

https://codereview.chromium.org/487363003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/socket/udp_node.cc:137:
filesystem()->ppapi()->ReleaseResource(addr_);
On 2014/08/19 20:46:59, noelallen1 wrote:
> While it is possible to call Start without calling Run, it shouldn't be
possible
> RecvFrom to populate addr_ without also calling the callback correct?

Thanks for reply!

If the completion callback is "required", then it will be run for sure. I just
searched the code for GetRunCompletion(). And you are right, it is a "required"
callback.

(Optional callback could provide better performance in some cases. Pepper can
choose to complete the call synchronously and skip the callback.)

> 
> 
> On 2014/08/19 18:37:29, yzshen1 wrote:
> > drive-by:
> > Maybe we could consider "safer" ref-counting.
> > - addr_ is not reset but this class doesn't own a ref after this
> > ReleaseResource() call.
> > - if Start() is called but Run() isn't, then addr_ is eventually leaked.
> > 
> > (I don't know much about this part of code base, though. Just my 2 cents.
:))
>

[binji, 2014-08-19 23:29:49]

Committed patchset #1 manually as 290691 (presubmit successful).