New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: unix,win: add uv_homedir() #350

Closed
wants to merge 1 commit into
base: v1.x
from

Conversation

Projects
None yet
4 participants
@cjihrig
Contributor

cjihrig commented May 12, 2015

This is a first pass at implementing a homedir() function, per the discussion in nodejs/node#1670.

@piscisaureus I'm running into a problem on the Windows side, where including shlobj.h (needed for SHGetKnownFolderPath()) is causing a ton of compiler errors. Any assistance would be much appreciated.

Also related to #11.

@saghul

This comment has been minimized.

Member

saghul commented May 12, 2015

Is this the only thing we need/want from struct passwd? I guess the username / user ID would also be nice, wouldn't it?

.. c:function:: int uv_homedir(char* home, size_t size)
Gets the current user's home directory. Calls `SHGetKnownFolderPath()` on
Windows, and `getlogin()` followed by `getpwnam()` otherwise.

This comment has been minimized.

@saghul

saghul May 12, 2015

Member

We can now document functions with automagic links to man pages:

"👨getaddrinfo(3)"

if (login == NULL)
return -errno;
pw = getpwnam((const char*) login);

This comment has been minimized.

@saghul

saghul May 12, 2015

Member

I think we better use the reentrant version, getpwnam_r.

@@ -983,3 +984,26 @@ int uv__dup2_cloexec(int oldfd, int newfd) {
return r;
}
}
int uv_homedir(char* home, size_t len) {

This comment has been minimized.

@saghul

saghul May 12, 2015

Member

For consistency with other similar functions, len should be a pointer, which passes in the size of the buffer, and get written with the amount if written data minus the trailing null. Check uv_exepath for example.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 12, 2015

Is this the only thing we need/want from struct passwd? I guess the username / user ID would also be nice, wouldn't it?

Yes, it would be nice, but we are limited by the Windows side. Separate calls are needed for all of these things - SHGetKnownFolderPath() gets the home directory, but we need to call GetUsernameW() to get the username. I'm open to doing that if that's the way everyone else wants to go.

@saghul

This comment has been minimized.

Member

saghul commented May 12, 2015

Yes, it would be nice, but we are limited by the Windows side. Separate calls are needed for all of these things - SHGetKnownFolderPath() gets the home directory, but we need to call GetUsernameW() to get the username. I'm open to doing that if that's the way everyone else wants to go.

I don't know, just wondering :-) Maybe some uv_passwd_t, with the common subset of things would be a good thing. On Unix we could fill in more things than on Windows.

As a side note, Python does this: https://docs.python.org/2/library/getpass.html#getpass.getuser that is, it reads the environment variables to get the username. Doesn't sound very reliable though.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 12, 2015

@saghul I updated the PR based on your comments, minus moving to the uv_passwd_t format. Regarding Python, I think that we should avoid relying on environment variables if possible.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 15, 2015

if (hr != S_OK)
return uv_translate_sys_error(WSAGetLastError());
r = wcstombs(home, path, len);

This comment has been minimized.

@piscisaureus

piscisaureus May 15, 2015

Member
  • use len - 1 because otherwise adding the null terminator (couple lines below) could cause a buffer overrun.
  • I'd prefer to use WideCharToMultiByte instead of wcstombs like we do everywhere in libuv.
if (result == NULL)
return -ENOENT;
strncpy(home, (const char*) pw.pw_dir, *len);

This comment has been minimized.

@piscisaureus

piscisaureus May 15, 2015

Member

Use after free? The buffer that stores this information is released a couple of lines up.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 15, 2015

@piscisaureus sorry I should have added this to my ping. Please see my message when I originally opened the PR. I'm having problems with Windows and including shlobj.h. The Windows code isn't ready for a thorough review yet.

return uv_translate_sys_error(WSAGetLastError());
home[r] = '\0';
*len = r;

This comment has been minimized.

@piscisaureus

piscisaureus May 15, 2015

Member

What's implied here is that the returned length doesn't include the null terminator.
I have no particular opinion on whether that's good or bad, but please ensure that this API behaves the same as the unix version, and be consistent with other path-returning functions (e.g. uv_cwd())

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented May 15, 2015

I'm having problems with Windows and including shlobj.h

Just make sure shlobj.h is included after windows.h.
Since windows.h is pulled in by uv.h you want to move it after uv.h.

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented May 15, 2015

Some other comments:

  • The windows implementation should live in util.c (where utility functions live) and not core.c (which contains the core event loop driver).
  • It'd be good to make sure trailing (back)slashes are included (or excluded) consistently across supported platforms. This has caused issues in the past with uv_cwd().
@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 16, 2015

@piscisaureus thank you for the help. I've updated the PR based on your comments. The unix and windows branches both ignore the null terminator when returning the length. Also, in my testing I didn't encounter any trailing slashes on the home directories. The SHGetKnownFolderPath() documentation explicitly states that a trailing slash is not included.

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented May 16, 2015

@cjihrig Great.

The windows bits lgtm.

I guess @bnoordhuis or @saghul could review the unix implementation. I'm a bit surprised that looking up the home dir requires retrieving the username first - looks like getpw(3) just provides the right information - but then I may be overlooking something.

@saghul

This comment has been minimized.

Member

saghul commented May 16, 2015

I'm traveling ATM, I'll have a look on Monday, if Ben doesn't beat me to it
:-)
On May 16, 2015 03:09, "Bert Belder" notifications@github.com wrote:

@cjihrig https://github.com/cjihrig Great.

The windows bits lgtm. I guess @bnoordhuis https://github.com/bnoordhuis
or @saghul https://github.com/saghul could review the windows bits -
I'm a bit surprised that looking up the home dir requires retrieving the
username first - looks like getpw(3) just provides the right information

  • but then I'm may be overlooking something.


Reply to this email directly or view it on GitHub
#350 (comment).

@@ -52,6 +53,9 @@
# include <mach-o/dyld.h> /* _NSGetExecutablePath */
# include <sys/filio.h>
# include <sys/ioctl.h>
# ifndef LOGIN_NAME_MAX
# define LOGIN_NAME_MAX _POSIX_LOGIN_NAME_MAX

This comment has been minimized.

@bnoordhuis

bnoordhuis May 16, 2015

Member

This looks incorrect to me. Most platforms leave _POSIX_LOGIN_NAME_MAX at the POSIX default of 9 (8 characters + zero byte) even though their real maximum is much larger.

if (buf == NULL)
return -ENOMEM;
r = getpwnam_r((const char*) login, &pw, buf, bufsize, &result);

This comment has been minimized.

@bnoordhuis

bnoordhuis May 16, 2015

Member

You should arguably use getpwuid_r() here, then you don't have to call getlogin_r() either. Retry with a bigger buffer when the call fails with ERANGE. (EDIT: Removed ambiguous errno reference.)

Note that sysconf(_SC_GETPW_R_SIZE_MAX) can (at least in theory) fail with -1. I suggest returning -EIO in that case.

return -ENOENT;
}
strncpy(home, (const char*) pw.pw_dir, *len);

This comment has been minimized.

@bnoordhuis

bnoordhuis May 16, 2015

Member

You don't have to const-cast the function parameter.

strncpy(home, (const char*) pw.pw_dir, *len);
free(buf);
home[*len - 1] = '\0';
*len = strlen(home);

This comment has been minimized.

@bnoordhuis

bnoordhuis May 16, 2015

Member

I would strlen() first, clamp the result, then memcpy() and add the zero byte if necessary.

strncpy() + strlen() is not wrong but strncpy() also unnecessarily zeros the part of the buffer after the string.

struct passwd* result;
char* buf;
size_t loginsize = sizeof(login);
size_t bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);

This comment has been minimized.

@bnoordhuis

bnoordhuis May 16, 2015

Member

Minor style issue: the code in src/unix does not normally use the C99 declare + assign syntax.

int uv_homedir(char* home, size_t* len) {

This comment has been minimized.

@bnoordhuis

bnoordhuis May 16, 2015

Member

Can you remove the blank line?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented May 16, 2015

I wonder if this should check $HOME first? It's something of a UNIX tradition to be able to change your home directory on the fly. A problem with that is that getenv("HOME") is not MT-safe.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 17, 2015

@bnoordhuis updated based on your comments.

@piscisaureus I added a new test case where the buffer is smaller than the length of the home directory string. This caused a problem on Windows with WideCharToMultiByte(). It doesn't truncate, but instead returns an error. How have you dealt with this in the past? If we wanted to match the Unix behavior, the home directory could be read into a larger buffer and then truncated. Thoughts?

UPDATE: Added 4c32c20 to handle small buffers on Windows if desired.

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented May 17, 2015

If we wanted to match the Unix behavior, the home directory could be read into a larger buffer and then truncated.

Are you saying that on unix the string is just silently truncated?

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 17, 2015

The unix code allocates a structure within uv_homedir() to read the data into. I haven't experimented with allocating any size other than sysconf(_SC_GETPW_R_SIZE_MAX). On Windows, I was calling WideCharToMultiByte() with the buffer allocated by the user. When this was too small in my test, a "buffer too small for system call" error was returned. I was able to create a workaround in 4c32c20 that determines how much space is needed, allocates a buffer, calls WideCharToMultiByte(), then does a memcpy() of the appropriate size.

@saghul

This comment has been minimized.

Member

saghul commented May 17, 2015

Some pseudorandom thought: should this be called uv_os_xxx? Kind of adding some name spacing...

if (buf == NULL)
return -ENOMEM;
r = getpwuid_r(getuid(), &pw, buf, bufsize, &result);

This comment has been minimized.

@bnoordhuis

bnoordhuis May 17, 2015

Member

You should retry on ERANGE here:

buf = NULL;
for (;;) {
  free(buf);

  buf = malloc(bufsize);
  if (buf == NULL)
    return -ENOMEM;

  err = getpwuid_r(uid, &pw, buf, bufsize, &result);
  if (err != ERANGE)
    break;

  bufsize *= 2;
}

As to whether uid should be the result of getuid() or geteuid(), I think getuid() is proper (it's what e.g. Ruby's Etc module defaults to), although I suppose it may be somewhat surprising for suid root binaries.

This comment has been minimized.

@cjihrig

cjihrig May 17, 2015

Contributor

Sorry I thought the comment about retrying was specific to getpwnam_r(). I'll update this.

memcpy(buffer, pw.pw_dir, len);
*size = len - 1;
buffer[*size] = '\0';

This comment has been minimized.

@bnoordhuis

bnoordhuis May 17, 2015

Member

You can simplify the string copy a little if you write it like this:

len = strlen(pw.pw_dir);
if (len >= *size)
  len = *size - 1;

memcpy(buffer, pw.pw_dir, len);
buffer[len] = '\0';
*size = len;
@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 18, 2015

Originally, I did not update len, but was asked to for consistency with other APIs. Should I undo that change?

I'm fine with not truncating. Should I make that change? If so, which error should I use?

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 19, 2015

@bnoordhuis @piscisaureus @saghul - I'm just waiting on a little clarification on my last comment. I'd like to mostly wrap this up tonight if possible (I'll be traveling the rest of the week).

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented May 19, 2015

None of the other APIs touch len on error. I'd opt for consistency now and perhaps revise in v2.0. Just return E2BIG if the result doesn't fit.

@saghul

This comment has been minimized.

Member

saghul commented May 19, 2015

@cjihrig Sorry about the delay! We seem to have a bit of a mix :-S Functions which set len to the desired length and return UV_ENOBUFS:

  • uv_pipe_get{sock,peer}name
  • uv_fs_{event,poll}_getpath
  • uv_cwd (on Windows)

Functions that truncate:

  • uv_exepath
  • uv_cwd (on Unix)

My personal preference would be to set len to the required value and return UV_ENOBUFS, a truncated path is kinda useless, IMHO. Ben said E2BIG would be more POSIX-y, but I think that boat has sailed...

correctness vs consistency, that is the question :-)

`GetEnvironmentVariableW()`. If `USERPROFILE` is not set,
`SHGetKnownFolderPath()` is called. On all other operating systems,
`uv_os_homedir()` first checks the `HOME` environment variable using
:man:getenv(3). If `HOME` is not set, :man:`getpwuid_r(3)` is called. The

This comment has been minimized.

@saghul

saghul May 19, 2015

Member

Backticks around getenv(3).

:man:getenv(3). If `HOME` is not set, :man:`getpwuid_r(3)` is called. The
user's home directory is stored in `buffer`. When `uv_os_homedir()` is
called, `size` indicates the maximum size of `buffer`. On success, `size` is
set to the string length of `buffer`. `uv_os_homedir()` is not thread safe.

This comment has been minimized.

@saghul

saghul May 19, 2015

Member

Can you please put the last sentence in a ..warning note block? (http://sphinx-doc.org/markup/para.html)

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 19, 2015

I don't have much say in how libuv behaves, but to me it seems that truncating is not helpful, and instead it would be better to return UV_ENOBUFS or E2BIG (your call), and set *size to the size required (pass or fail). I guess that would change the current success semantics where *size is set to the strlen() and not the actual size required.

/* Check if the USERPROFILE environment variable is set first */
bufsize = GetEnvironmentVariableW(L"USERPROFILE", NULL, 0);
if (bufsize == 0)

This comment has been minimized.

@saghul

saghul May 19, 2015

Member

Probably unlikely, but we should check GetLastError here, in case it's ERROR_ENVVAR_NOT_FOUND it's not an error, we should just continue. I see you handle it later, but I guess we need to check both times, because threads!

@saghul

This comment has been minimized.

Member

saghul commented May 19, 2015

None of the other APIs touch len on error. I'd opt for consistency now and perhaps revise in v2.0. Just return E2BIG if the result doesn't fit.

@bnoordhuis but they do!

https://github.com/libuv/libuv/blob/v1.x/src/uv-common.c#L373
https://github.com/libuv/libuv/blob/v1.x/src/unix/pipe.c#L239
https://github.com/libuv/libuv/blob/v1.x/src/win/util.c#L209

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 20, 2015

Another round of updates pushed :-)

@saghul

This comment has been minimized.

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented May 20, 2015

Great! let's establish some common patterns

@saghul

This comment has been minimized.

Member

saghul commented May 20, 2015

Build has tons of red, but that has nothing to do with this patch, great work @cjihrig!

.. warning::
`uv_os_homedir()` is not thread safe.

This comment has been minimized.

@saghul

saghul May 20, 2015

Member

One last thing (if you're not around I'll add it during merge) can you add a ".. versionadded 1.6.0" here please? :-)

This comment has been minimized.

@cjihrig

cjihrig May 20, 2015

Contributor

Added

@saghul

This comment has been minimized.

Member

saghul commented May 20, 2015

Fantastic! Can we get another LGTM? /cc @bnoordhuis, @piscisaureus

/* HOME is not set, so call getpwuid() */
errno = 0;
pw = getpwuid(getuid());

This comment has been minimized.

@bnoordhuis

bnoordhuis May 20, 2015

Member

I'd rather you stick to getpwuid_r() here. getenv() at least is MT-safe on most platforms if no other threads concurrently modify the environment, but getpwuid() is basically always unsafe.

This comment has been minimized.

@saghul

saghul May 20, 2015

Member

Oh I missed that during my review. Agreed!

This comment has been minimized.

@cjihrig

cjihrig May 21, 2015

Contributor

@bnoordhuis @saghul restored getpwuid_r().

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented May 21, 2015

LGTM

@saghul

This comment has been minimized.

Member

saghul commented May 21, 2015

Kewl, I'll merge then.

saghul added a commit that referenced this pull request May 21, 2015

unix,win: add uv_os_homedir()
PR-URL: #350
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@saghul

This comment has been minimized.

Member

saghul commented May 21, 2015

Great work Colin! Landed in a62c2d5

@saghul saghul closed this May 21, 2015

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 21, 2015

Thanks Saul/Ben/Bert for working with me through this.

@cjihrig cjihrig deleted the cjihrig:homedir branch May 21, 2015

guworks added a commit to guworks/libuv that referenced this pull request Jul 9, 2015

unix,win: add uv_os_homedir()
PR-URL: libuv#350
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

skomski added a commit to skomski/libuv that referenced this pull request Aug 5, 2015

unix,win: add uv_os_homedir()
PR-URL: libuv#350
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

skomski added a commit to skomski/libuv that referenced this pull request Aug 5, 2015

unix,win: add uv_os_homedir()
PR-URL: libuv#350
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment