Fix handling of free descriptors when calling dup2.

Fix handling of free descriptors when calling dup2.

We previously were not properly expanding the free list when
dup2 results in growing the descriptor range.

Added of test of this.

BUG=None
TEST=Dup2Allocate
R=binji@chromium.org

Committed: https://crrev.com/61814731fc7295e1586c7d36ebbd6c9308a4eccb
Cr-Commit-Position: refs/heads/master@{#308232}

[Sam Clegg, 2014-12-12 00:44:39]

sbc@chromium.org changed reviewers:
+ sbc@chromium.org

[Sam Clegg, 2014-12-12 00:44:40]

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:257: int sz =
(int)handle_map_.size();
Move this line outside the condition to avoid calling .size() twice?

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:258:
handle_map_.resize(fd + 1);
A comment here to describe what is happening and why it might happen?

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/tests/...
File native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:530:
TEST_F(KernelProxyTest, Dup2Allocation) {
Comment here?   I would like a better test name but I can see its hard to come
up with a short and more meaningful one.

[binji, 2014-12-12 18:55:20]

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:256: if (fd >=
(int)handle_map_.size()) {
use c++-style cast

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/tests/...
File native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:535: ASSERT_EQ(fd
+ 1, dup_fd);
Is this guaranteed to be true? The fd returned should always be the lowest
available, right? In that case, if 3 handles are opened, and the middle one is
closed, then this would be false, correct?

Maybe it would be safer to allocate a bunch of fds until we get three in a row,
at which point those can be released, and we know it's safe to continue the
test.

Or am I just being paranoid?

[bradn, 2014-12-12 22:58:18]

ptal

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:256: if (fd >=
(int)handle_map_.size()) {
On 2014/12/12 18:55:20, binji wrote:
> use c++-style cast

Done.

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:257: int sz =
(int)handle_map_.size();
On 2014/12/12 00:44:40, Sam Clegg wrote:
> Move this line outside the condition to avoid calling .size() twice?

Done.

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:258:
handle_map_.resize(fd + 1);
On 2014/12/12 00:44:40, Sam Clegg wrote:
> A comment here to describe what is happening and why it might happen?

Done.

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/tests/...
File native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:530:
TEST_F(KernelProxyTest, Dup2Allocation) {
On 2014/12/12 00:44:40, Sam Clegg wrote:
> Comment here?   I would like a better test name but I can see its hard to come
> up with a short and more meaningful one.


Added more.

https://codereview.chromium.org/795213003/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:535: ASSERT_EQ(fd
+ 1, dup_fd);
On 2014/12/12 18:55:20, binji wrote:
> Is this guaranteed to be true? The fd returned should always be the lowest
> available, right? In that case, if 3 handles are opened, and the middle one is
> closed, then this would be false, correct?
> 
> Maybe it would be safer to allocate a bunch of fds until we get three in a
row,
> at which point those can be released, and we know it's safe to continue the
> test.
> 
> Or am I just being paranoid?

Commented more verbosely.
The test assumes the initial descriptor set starts from zero and is contiguous.
I suppose that might strictly be true (though if anything I would imagine posix
probably mandates something about only 0-2 being initially allocated.
I've commented more extensively.
The trouble with trying to find an contiguous region is that the fix this is
meant to cover might have hidden behind allocating the descriptors one by one
(as then they'd correctly be in the free list).
I suppose grabbing bigger chunks might work, but that seems overkill.

[binji, 2014-12-13 00:55:30]

lgtm

[bradn, 2014-12-13 01:10:18]

The CQ bit was checked by bradnelson@google.com

[commit-bot: I haz the power, 2014-12-13 01:12:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/795213003/20001

[commit-bot: I haz the power, 2014-12-13 02:55:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-13 02:56:03]

Patchset 2 (id:??) landed as
https://crrev.com/61814731fc7295e1586c7d36ebbd6c9308a4eccb
Cr-Commit-Position: refs/heads/master@{#308232}