cryptohome: Add new platform functions

cryptohome: Add new platform functions

This is first in a series of CLs to add opencryptoki initialization to cryptohome.
This ones adds new platform functions for
1) changing file/path ownership recursively.
2) creating symlinks.
3) getting group id on a path.
4) executing a binary as a specific effective uid/gid.

(Original patchset by fes@chromium.org. This ones fixes some
minor bugs and contains documentation fixes.)

Change-Id: I424ca11fcb74ca5d3b040dccf0cf82ed8da766cc

BUG=chromium-os:12295
TEST=emerge-x86-mario chromeos-base/chromeos-cryptohome

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=6841c9e

[gauravsh, 2011-03-24 01:51:28]

Updated issue description with a note about the original provenance of the
patch.

[Will Drewry, 2011-03-25 21:02:08]

LGTM if setresuid/gid ordering is correct.

Thanks for splitting this out.  A few comments on the execute path, but nothing
blocking.  All style and logging.

http://codereview.chromium.org/6729012/diff/1/platform.cc
File platform.cc (right):

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode365
platform.cc:365: file_util::FileEnumerator::DIRECTORIES);
Any idea if this magically follows symlink dirs too?

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode416
platform.cc:416: if(group_name_length == -1) {
style nit: if (

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode444
platform.cc:444: LOG(ERROR) << "Error creating symbolic link from " << from << "
to " << to
PLOG will give the errno conversion too.

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode463
platform.cc:463: if (gid != static_cast<gid_t>(-1)) {
Does setresuid need to be called last?

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode475
platform.cc:475: execve(command.c_str(), const_cast<char* const*>(local_args),
NULL);
Worth a PLOG(ERROR) here if execve fails?

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode487
platform.cc:487: }
If not, we could check for exit status 3 here.

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode489
platform.cc:489: return false;
Might be worth doing a PLOG() before return false to find out why vfork failed.

[gauravsh, 2011-03-25 23:30:46]

PTAL.

http://codereview.chromium.org/6729012/diff/1/platform.cc
File platform.cc (right):

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode365
platform.cc:365: file_util::FileEnumerator::DIRECTORIES);
On 2011/03/25 21:02:09, Will Drewry wrote:
> Any idea if this magically follows symlink dirs too?

I looked at the source, and unless the 4th argument has
file_util::FileEnumerator::SHOW_SYM_LINKS flags, it shouldn't. That's my reading
of it (the code is in file_util_posix.cc).

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode416
platform.cc:416: if(group_name_length == -1) {
On 2011/03/25 21:02:09, Will Drewry wrote:
> style nit: if (

Done.

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode444
platform.cc:444: LOG(ERROR) << "Error creating symbolic link from " << from << "
to " << to
On 2011/03/25 21:02:09, Will Drewry wrote:
> PLOG will give the errno conversion too.

Done.

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode463
platform.cc:463: if (gid != static_cast<gid_t>(-1)) {
On 2011/03/25 21:02:09, Will Drewry wrote:
> Does setresuid need to be called last?

I do not think it matters. (Didn't find anything in the man page either.) I have
reversed the order just in case.

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode475
platform.cc:475: execve(command.c_str(), const_cast<char* const*>(local_args),
NULL);
On 2011/03/25 21:02:09, Will Drewry wrote:
> Worth a PLOG(ERROR) here if execve fails? 

Done.

http://codereview.chromium.org/6729012/diff/1/platform.cc#newcode489
platform.cc:489: return false;
On 2011/03/25 21:02:09, Will Drewry wrote:
> Might be worth doing a PLOG() before return false to find out why vfork
failed.

Done.

[Will Drewry, 2011-03-26 01:06:05]

LGTM with an optional log message clean up.

thanks!

http://codereview.chromium.org/6729012/diff/3002/platform.cc
File platform.cc (right):

http://codereview.chromium.org/6729012/diff/3002/platform.cc#newcode490
platform.cc:490: PLOG(ERROR) << "Couldn't spawn a subprocess for command
execution.";
We might also see this if the exit status is non-zero.  Not sure if it's worth
clarifying or just making this more generic and adding a note when the
WEXITSTATUS(status) is non-zero (and emitting it).  Your call!

[gauravsh, 2011-03-26 23:38:40]

http://codereview.chromium.org/6729012/diff/3002/platform.cc
File platform.cc (right):

http://codereview.chromium.org/6729012/diff/3002/platform.cc#newcode490
platform.cc:490: PLOG(ERROR) << "Couldn't spawn a subprocess for command
execution.";
On 2011/03/26 01:06:05, Will Drewry wrote:
> We might also see this if the exit status is non-zero.  Not sure if it's worth
> clarifying or just making this more generic and adding a note when the
> WEXITSTATUS(status) is non-zero (and emitting it).  Your call!

I added another PLOG in the inner if-block (where the status gets checked) and
moved this PLOG to an else{} block.