You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/12/22 20:50:26 UTC
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/
-----------------------------------------------------------
(Updated Dec. 22, 2016, 8:50 p.m.)
Review request for mesos and Alexander Rukletsov.
Changes
-------
New approach, per discussion with AlexR.
Summary (updated)
-----------------
Made `getpwnam_r` error handling more robust.
Bugs: MESOS-6826
https://issues.apache.org/jira/browse/MESOS-6826
Repository: mesos
Description (updated)
-------
According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
null pointer) when the specified user name is not found. However,
certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
non-zero and set `errno` (to one of several different values) when
`getpwnam_r` is passed an invalid user name.
In stout, we want to treat the "invalid user name" and "user name not
found" cases the same. Both the POSIX spec and Linux manpages call out
certain errno values as definitely indicating errors (e.g., EIO,
EMFILE). On Linux, we check `errno` and return an error to the caller if
`errno` appears in that list. We treat `errno` values not in that list
as equivalent to "user not found".
On other (Unix) platforms, we treat any non-zero return value from
`getpwnam_r` as indicating an error.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
Diff: https://reviews.apache.org/r/54952/diff/
Testing
-------
`make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
Thanks,
Neil Conway
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 23, 2016, 11:08 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/os/posix/su.hpp, line 58
> > <https://reviews.apache.org/r/54952/diff/2/?file=1591277#file1591277line58>
> >
> > We'll check `result != nullptr` later; could you initialize it here for robustness?
I don't think this is an improvement, because it obscures the fact that (a) `result` is only accessed _after_ `getpwnam_r` is called, and (b) `getpwnam_r` always writes-through to the `result` pointer. Leaving the variable uninitialized lets the compiler warn us if the code is changed to violate these properties.
> On Dec. 23, 2016, 11:08 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/os/posix/su.hpp, line 104
> > <https://reviews.apache.org/r/54952/diff/2/?file=1591277#file1591277line104>
> >
> > Checking for `__linux__` here seems wrong as we care about a feature of the used C stdlib, not the platform.
> >
> > If I understand this code and the comment above it correctly, we should never enter this `if` branch with a POSIX-conformant stdlib, but could potentially if e.g., glibc is being used.
> >
> > We should either fix this to (a) use the correct feature check macro checking for the used C stdlib instead of the platform, or since taking this branch is impossible with a POSIX-conformant stdlib, (b) just unconditionally enable this sanity check (i.e., drop the `ifdef`). I lean towards the second option as it would add robustness to this wrapper to deal with other non-conformant stdlibs.
Fair point. I'd lean towards dropping the `ifdef` is probably better as well.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/#review160066
-----------------------------------------------------------
On Dec. 22, 2016, 8:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54952/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2016, 8:50 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6826
> https://issues.apache.org/jira/browse/MESOS-6826
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
> null pointer) when the specified user name is not found. However,
> certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
> non-zero and set `errno` (to one of several different values) when
> `getpwnam_r` is passed an invalid user name.
>
> In stout, we want to treat the "invalid user name" and "user name not
> found" cases the same. Both the POSIX spec and Linux manpages call out
> certain errno values as definitely indicating errors (e.g., EIO,
> EMFILE). On Linux, we check `errno` and return an error to the caller if
> `errno` appears in that list. We treat `errno` values not in that list
> as equivalent to "user not found".
>
> On other (Unix) platforms, we treat any non-zero return value from
> `getpwnam_r` as indicating an error.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
> 3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
>
> Diff: https://reviews.apache.org/r/54952/diff/
>
>
> Testing
> -------
>
> `make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/#review160066
-----------------------------------------------------------
3rdparty/stout/include/stout/os/posix/su.hpp (line 55)
<https://reviews.apache.org/r/54952/#comment231063>
We'll check `result != nullptr` later; could you initialize it here for robustness?
3rdparty/stout/include/stout/os/posix/su.hpp (line 93)
<https://reviews.apache.org/r/54952/#comment231064>
Checking for `__linux__` here seems wrong as we care about a feature of the used C stdlib, not the platform.
If I understand this code and the comment above it correctly, we should never enter this `if` branch with a POSIX-conformant stdlib, but could potentially if e.g., glibc is being used.
We should either fix this to (a) use the correct feature check macro checking for the used C stdlib instead of the platform, or since taking this branch is impossible with a POSIX-conformant stdlib, (b) just unconditionally enable this sanity check (i.e., drop the `ifdef`). I lean towards the second option as it would add robustness to this wrapper to deal with other non-conformant stdlibs.
- Benjamin Bannier
On Dec. 22, 2016, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54952/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2016, 9:50 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6826
> https://issues.apache.org/jira/browse/MESOS-6826
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
> null pointer) when the specified user name is not found. However,
> certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
> non-zero and set `errno` (to one of several different values) when
> `getpwnam_r` is passed an invalid user name.
>
> In stout, we want to treat the "invalid user name" and "user name not
> found" cases the same. Both the POSIX spec and Linux manpages call out
> certain errno values as definitely indicating errors (e.g., EIO,
> EMFILE). On Linux, we check `errno` and return an error to the caller if
> `errno` appears in that list. We treat `errno` values not in that list
> as equivalent to "user not found".
>
> On other (Unix) platforms, we treat any non-zero return value from
> `getpwnam_r` as indicating an error.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
> 3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
>
> Diff: https://reviews.apache.org/r/54952/diff/
>
>
> Testing
> -------
>
> `make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/#review160335
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54952]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 28, 2016, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54952/
> -----------------------------------------------------------
>
> (Updated Dec. 28, 2016, 9:50 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6826
> https://issues.apache.org/jira/browse/MESOS-6826
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
> null pointer) when the specified user name is not found. However,
> certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
> non-zero and set `errno` (to one of several different values) when
> `getpwnam_r` is passed an invalid user name.
>
> In stout, we want to treat the "invalid user name" and "user name not
> found" cases the same. Both the POSIX spec and Linux manpages call out
> certain errno values as definitely indicating errors (e.g., EIO,
> EMFILE). Hence, we check `errno` and return an error to the caller if
> `errno` appears in that list. We treat `errno` values not in that list
> as equivalent to "user not found".
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
> 3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
>
> Diff: https://reviews.apache.org/r/54952/diff/
>
>
> Testing
> -------
>
> `make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/#review160595
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On Dec. 28, 2016, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54952/
> -----------------------------------------------------------
>
> (Updated Dec. 28, 2016, 9:50 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6826
> https://issues.apache.org/jira/browse/MESOS-6826
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
> null pointer) when the specified user name is not found. However,
> certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
> non-zero and set `errno` (to one of several different values) when
> `getpwnam_r` is passed an invalid user name.
>
> In stout, we want to treat the "invalid user name" and "user name not
> found" cases the same. Both the POSIX spec and Linux manpages call out
> certain errno values as definitely indicating errors (e.g., EIO,
> EMFILE). Hence, we check `errno` and return an error to the caller if
> `errno` appears in that list. We treat `errno` values not in that list
> as equivalent to "user not found".
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
> 3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
>
> Diff: https://reviews.apache.org/r/54952/diff/
>
>
> Testing
> -------
>
> `make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/
-----------------------------------------------------------
(Updated Dec. 28, 2016, 9:50 p.m.)
Review request for mesos and Alexander Rukletsov.
Changes
-------
Address review comments. Make `os::user` consistent with changes to `os::getuid` and `os::getgid`.
Bugs: MESOS-6826
https://issues.apache.org/jira/browse/MESOS-6826
Repository: mesos
Description (updated)
-------
According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
null pointer) when the specified user name is not found. However,
certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
non-zero and set `errno` (to one of several different values) when
`getpwnam_r` is passed an invalid user name.
In stout, we want to treat the "invalid user name" and "user name not
found" cases the same. Both the POSIX spec and Linux manpages call out
certain errno values as definitely indicating errors (e.g., EIO,
EMFILE). Hence, we check `errno` and return an error to the caller if
`errno` appears in that list. We treat `errno` values not in that list
as equivalent to "user not found".
Diffs (updated)
-----
3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
Diff: https://reviews.apache.org/r/54952/diff/
Testing
-------
`make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
Thanks,
Neil Conway
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 23, 2016, 10:40 a.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/os/posix/su.hpp, line 66
> > <https://reviews.apache.org/r/54952/diff/2/?file=1591277#file1591277line66>
> >
> > How about pulling `delete[] buffer` before the comment because it is common to both found/not found cases (similar to what you do for the `else` case below)?
> >
> > Ideally we should use something like `BOOST_SCOPE_EXIT` or `std::vector` in combination with `vector::reserve()`, but this is probably should be done consistenly across stout.
Now when you extended the patch to `os::user` I see that we use `pwd.pw_name` there and hence cannot deallocate the buffer at the beginning of the if-clause. It probably makes sense to leave things as is for local consistency. Dropping the issue.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/#review160065
-----------------------------------------------------------
On Dec. 28, 2016, 9:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54952/
> -----------------------------------------------------------
>
> (Updated Dec. 28, 2016, 9:50 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6826
> https://issues.apache.org/jira/browse/MESOS-6826
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
> null pointer) when the specified user name is not found. However,
> certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
> non-zero and set `errno` (to one of several different values) when
> `getpwnam_r` is passed an invalid user name.
>
> In stout, we want to treat the "invalid user name" and "user name not
> found" cases the same. Both the POSIX spec and Linux manpages call out
> certain errno values as definitely indicating errors (e.g., EIO,
> EMFILE). Hence, we check `errno` and return an error to the caller if
> `errno` appears in that list. We treat `errno` values not in that list
> as equivalent to "user not found".
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
> 3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
>
> Diff: https://reviews.apache.org/r/54952/diff/
>
>
> Testing
> -------
>
> `make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 54952: Made `getpwnam_r` error handling more
robust.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54952/#review160065
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/include/stout/os/posix/su.hpp (line 54)
<https://reviews.apache.org/r/54952/#comment231058>
Since you touch this line, I'd suggest renaming the variable name so that it differs from the type name.
3rdparty/stout/include/stout/os/posix/su.hpp (line 63)
<https://reviews.apache.org/r/54952/#comment231059>
How about pulling `delete[] buffer` before the comment because it is common to both found/not found cases (similar to what you do for the `else` case below)?
Ideally we should use something like `BOOST_SCOPE_EXIT` or `std::vector` in combination with `vector::reserve()`, but this is probably should be done consistenly across stout.
- Alexander Rukletsov
On Dec. 22, 2016, 8:50 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54952/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2016, 8:50 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-6826
> https://issues.apache.org/jira/browse/MESOS-6826
>
>
> Repository: mesos
>
>
> Description
> -------
>
> According to POSIX, `getpwnam_r` returns 0 (and sets `result` to the
> null pointer) when the specified user name is not found. However,
> certain versions of Linux (e.g., RHEL7, recent Arch Linux) return
> non-zero and set `errno` (to one of several different values) when
> `getpwnam_r` is passed an invalid user name.
>
> In stout, we want to treat the "invalid user name" and "user name not
> found" cases the same. Both the POSIX spec and Linux manpages call out
> certain errno values as definitely indicating errors (e.g., EIO,
> EMFILE). On Linux, we check `errno` and return an error to the caller if
> `errno` appears in that list. We treat `errno` values not in that list
> as equivalent to "user not found".
>
> On other (Unix) platforms, we treat any non-zero return value from
> `getpwnam_r` as indicating an error.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/posix/su.hpp f3f45ebf006f0f8e7e70b0f4c4ed76465530c58e
> 3rdparty/stout/tests/os_tests.cpp 8d2005b1f109b4025aa8368600763db9c829d0c5
>
> Diff: https://reviews.apache.org/r/54952/diff/
>
>
> Testing
> -------
>
> `make check` on Arch Linux. `OsTest.User` fails without this patch but succeeds with this patch.
>
>
> Thanks,
>
> Neil Conway
>
>