[NaCl SDK] Map active fds to absolute paths.

[NaCl SDK] Map active fds to absolute paths.

This implements both fchdir and adds an absolute path to the
Descriptor_t struct.  Typically on Linux systems this information can be
obtained by querying the /proc file system, but in absence of that, we
can track it ourselves.  For pipes and sockets, the path is the empty
string.

By enabling this behavior, fchdir can also be implemented.  This also
returns expected errno values from fchdir.  This opens up the ability to
do relative path operations like openat.  While these currently can be
executed in user code in a race condition prone way, they could be moved
into the KernelProxy, where the absolute paths can be both accessed and
resolved with AcquireHandleAndPath.

R=binji@chromium.org, sbc@chromium.org
BUG=

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

[Sam Clegg, 2014-01-06 18:52:49]

looks good, thanks!

I'll leave to to binji to give the final OK.

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:236:
fd_paths_.erase(fd);
Would this be better done as part of FreeFD()?

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:545: // errno set by
SetCWD
Are you sure?  I don't see SetCWD setting errno.

[Sam Clegg, 2014-01-06 18:55:55]

Also, please at at least one test to nacl_io_test.

You can run the tests outside of chrome (under sel_ldr) by
running the following command:

nacl_io_test $ make run SEL_LDR=1 NACL_ARCH=x86_64

[binji, 2014-01-06 19:32:04]

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:176: fd_paths_[fd] =
GetAbsParts(path).Join();
this probably should be handled in AllocateFD. fd_paths_ should be protected by
a lock (maybe handle_lock_?), so you'll likely need to add additional parameters
to AllocateFD (or create some new functions that take path/FD to copy path
from).

Hope this makes sense. :)

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:236:
fd_paths_.erase(fd);
On 2014/01/06 18:52:50, Sam Clegg wrote:
> Would this be better done as part of FreeFD()?

agreed.

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:545: // errno set by
SetCWD
On 2014/01/06 18:52:50, Sam Clegg wrote:
> Are you sure?  I don't see SetCWD setting errno.

Only the KernelProxy sets errno. KernelObject::SetCWD returns the error.

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.h (right):

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.h:202: FdPathMap_t
fd_paths_;
this should probably be in KernelObject

[Matthew Turk, 2014-01-07 20:30:54]

On 2014/01/06 19:32:04, binji wrote:
>
https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):
> 
>
https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:176: fd_paths_[fd] =
> GetAbsParts(path).Join();
> this probably should be handled in AllocateFD. fd_paths_ should be protected
by
> a lock (maybe handle_lock_?), so you'll likely need to add additional
parameters
> to AllocateFD (or create some new functions that take path/FD to copy path
> from).
> 
> Hope this makes sense. :)
> 
>
https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:236:
> fd_paths_.erase(fd);
> On 2014/01/06 18:52:50, Sam Clegg wrote:
> > Would this be better done as part of FreeFD()?
> 
> agreed.
> 
>
https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:545: // errno set by
> SetCWD
> On 2014/01/06 18:52:50, Sam Clegg wrote:
> > Are you sure?  I don't see SetCWD setting errno.
> 
> Only the KernelProxy sets errno. KernelObject::SetCWD returns the error.
> 
>
https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/kernel_proxy.h (right):
> 
>
https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_proxy.h:202: FdPathMap_t
> fd_paths_;
> this should probably be in KernelObject

Thanks for the comments!  I've moved the operations into KernelObject and added
tests for ki_fchdir, as well as checking the path inside the tests for
KernelObject.

[Matthew Turk, 2014-01-07 20:33:10]

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/99203014/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:176: fd_paths_[fd] =
GetAbsParts(path).Join();
On 2014/01/06 19:32:04, binji wrote:
> this probably should be handled in AllocateFD. fd_paths_ should be protected
by
> a lock (maybe handle_lock_?), so you'll likely need to add additional
parameters
> to AllocateFD (or create some new functions that take path/FD to copy path
> from).
> 
> Hope this makes sense. :)


It does -- and I've gone through and put path as an argument to AllocateFD in
the updated cl.

I did have one question, which was how to handle the potential race conditions
for relative paths being supplied to open() and the CWD changing between the
call to open() and a resolution of the path to an absolute path.

One possible solution is to perform the absolute path resolution inside
KernelProxy (where it happens now) and ensure that the path supplied to
AllocateFD is normalized and absolute, which would result in two sequential
locks (first cwd_ during path normalization and then handle_lock_).  I think
this only applies to open_resource() and open(), as in the new cl they're the
only ones that won't be handling a path that has already been normalized.  Or am
I worrying too much about this?

[binji, 2014-01-07 21:40:29]

Looking good.

I wouldn't worry too much about races with CWD; AFAICT it is inherently racy. As
long as we don't end up with inconsistencies, I think it's fine.

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:159: if (iter ==
fd_paths_.end()) {
nit: no braces for single-line ifs

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:160: return "";
return std::string();

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:215: fd_paths_[id] =
abs_path;
nit: fs_paths_[id] = GetAbsParts(path).Join();

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:230: handle_map_[fd] =
Descriptor_t(handle);
assign to fd_paths_[fd] here too (used by dup2)

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:175: int fd =
AllocateFD(handle, path);
return AllocateFD(handle, path);

is fine. Here and below

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:214: pipefds[0] =
AllocateFD(handle0, "");
I think I'd prefer having the second parameter default to std::string(), and
making this just AllocateFD(handle0);

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:246: std::string path =
GetFDPath(oldfd);
hm, I think there's a race here. The oldfd mapping can change between this call
and the handle acquired above from oldfd; this will make the dup'd fd have a
mismatched handle and path.

Here's a thought: add KernelObject::AcquireHandleAndPath that retrieves both in
the same locking of handle_lock_. You can use the same call below in fchdir.

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/99203014/diff/90001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:318:
EXPECT_STREQ("/foo/bar", text);
can you add tests for using fchdir on a fd that was created with dup and dup2?

[Matthew Turk, 2014-01-07 23:04:29]

Thanks for the comments!  I've updated to get rid of the race condition,
expanded the tests to include dup and dup2, and removed GetFDPath since it was
only used in operations prone to race conditions.

[Sam Clegg, 2014-01-07 23:56:39]

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_object.cc (right):

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:193: std::string
&out_path){
Put '&' next to the type according to the local style.

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:210: std::string path)
{
Why not pass string by reference here too?

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.cc:236: const std::string
&path) {
& next to type name.

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:40: typedef
std::map<int, std::string> FdPathMap_t;
Actually, why not just add at std::string to Descriptor_t?  Avoid this extra map
completely?

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/te...
File native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/99203014/diff/180001/native_client_sdk/src/te...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:334: 
nit: Remove this empty line.

[Matthew Turk, 2014-01-08 03:05:02]

This patchset removes fd_paths_ and moves the absolute path into the
Descriptor_t struct.  Thanks for suggesting this, Sam, I think it's a more
elegant solution than the double map.  This also removes GetFDPath, and the
relevant tests, as it's no longer necessary or desirable.  I've updated the
style, and also attempted to adhere to the G C++ style guide in the new code,
but if there are any additional style or code comments I'm eager to address
them.  Thanks very much!

[binji, 2014-01-08 17:25:43]

much better! A few nits, otherwise lgtm

https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):

https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:33: Descriptor_t() :
flags(0), path() {}
nit: path() is unnecessary, the default constructor will be used without it.

https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:71: std::string
GetFDPath(int fd);
remove GetFDPath

https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:79: std::string& path);
Our style guide says that out parameters must be pointers. Reference parameters
should always be const.

[binji, 2014-01-08 17:28:09]

On 2014/01/08 17:25:43, binji wrote:
> much better! A few nits, otherwise lgtm
> 
>
https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
> File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):
> 
>
https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
> native_client_sdk/src/libraries/nacl_io/kernel_object.h:33: Descriptor_t() :
> flags(0), path() {}
> nit: path() is unnecessary, the default constructor will be used without it.
> 
>
https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
> native_client_sdk/src/libraries/nacl_io/kernel_object.h:71: std::string
> GetFDPath(int fd);
> remove GetFDPath
> 
>
https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
> native_client_sdk/src/libraries/nacl_io/kernel_object.h:79: std::string&
path);
> Our style guide says that out parameters must be pointers. Reference
parameters
> should always be const.

Oh, and please update your CL description to linewrap at 72 or 80 characters.
You can use "git cl desc" to open the description in your editor.

[Matthew Turk, 2014-01-08 17:47:37]

I've changed the out parameter to be pass-by-pointer, addressed the other items,
run the sel_ldr tests, and fixed the cl desc.  Thank you!

[Sam Clegg, 2014-01-08 18:23:24]

lgtm

Can you add [NaCl SDK] at the beginning of the first
line of the CL description?

Once you addressed the final issues you should be able
to tick the "Commit" box and the change will go through
the commit queue.

Remember to land https://codereview.chromium.org/122943005/
first so you are in the AUTHORS file.

https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_object.h (right):

https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:35: : handle(h),
flags(0), path(path_) {}
The _ suffix is used for member variables in the style guide so
its a little confusing to use it for an argument here.

https://codereview.chromium.org/99203014/diff/250001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_object.h:85: const std::string&
path = std::string());
No spaces around the "=" sign for default params.

[commit-bot: I haz the power, 2014-01-10 19:18:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewturk@gmail.com/99203014/400001

[commit-bot: I haz the power, 2014-01-10 22:44:07]

Change committed as 244267