fix wrong definitions of getpwent_r and getgrent_r on solarish os #2914

SteveLauC · 2022-09-18T03:24:36Z

You may find the definitions for getpwnam_r/getpwuid_r/getgrnam_r/getgruid_r exposed by libc are also wrong:

struct passwd *getpwnam_r(const char *name, struct passwd *pwd,
            char *buffer, int buflen);

pub fn getpwnam_r(
    name: *const ::c_char,
    pwd: *mut passwd,
    buf: *mut ::c_cha
    buflen: ::size_t,
    result: *mut *mut passwd,
) -> ::c_int;

But actually they are correct as there are the POSIX-conforming definitions (see Standard conforming section of above man pages):

Standard conforming

       cc [ flag...] file... -D_POSIX_PTHREAD_SEMANTICS [ library... ]

       int getpwnam_r(const char *name, struct passwd *pwd, char *buffer,
            size_t bufsize, struct passwd **result);


       int getpwuid_r(uid_t uid, struct passwd *pwd, char *buffer,
            size_t bufsize, struct passwd **result);

getpwent_r/getgrent_r don't get lucky, they do not have the POSIX-conforming alternatives.

To double check this, I searched its source code:

$ rg "__posix_getpwnam_r"
port/mapfile-vers
1582:   __posix_getpwnam_r;

port/gen/getpwnam_r.c
152:__posix_getpwnam_r(const char *name, struct passwd *pwd, char *buffer,

$ rg "__posix_getpwent_r"

$

rust-highfive · 2022-09-18T03:24:38Z

Some changes occurred in solarish module

cc @jclulow,@pfmooney

rust-highfive · 2022-09-18T03:24:39Z

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

JohnTitor · 2022-09-19T07:29:41Z

This is a breaking change, did you make sure that the change doesn't break any code?

cc @ctrlcctrlv who originally added this

ctrlcctrlv · 2022-09-19T13:07:32Z

I mean, I have not tested personally on Solaris, but it looks right to me.

SteveLauC · 2022-09-21T01:04:34Z

This is a breaking change, did you make sure that the change doesn't break any code?

Not sure about this. So what are we going to do about this PR?

jclulow · 2022-09-21T01:18:48Z

I think @pfmooney and I will take a look at the specifics and make a recommendation, at least wrt. illumos.

JohnTitor · 2022-09-21T08:48:23Z

Not sure about this. So what are we going to do about this PR?

We focus on back-compat and changing declarations will break existing code (unless it's totally wrong and doesn't work at all), we have to confirm that the change here doesn't confuse anyone.

ctrlcctrlv · 2022-09-23T16:02:38Z

I'm to blame for this via #934. I have two questions.

Why isn't the right solution to add POSIX conforming versions of these functions to Illumos?
Why did CI pass even though __posix_get(pw|gr)ent_r doesn't exist?

ctrlcctrlv · 2022-10-01T12:31:33Z

Well, failing an answer to №2, I've gotten OmniOS (an Illumos distribution) working on my own. I suggest I just author a patch to Illumos to add the missing functions, making this issue moot. Does anyone have an opinion on that course of action?

SteveLauC · 2022-10-01T23:27:30Z

Does anyone have an opinion on that course of action?

Sounds good to me:)

jclulow · 2022-10-01T23:45:04Z

I've had a look at this, and I suspect that the right call is probably to implement, in Rust, the behaviour you're expecting based on the other platforms as a wrapper around the native function. I don't believe there's any substantial difference otherwise, so you could presumably do something a bit like this:

extern "C" {
    #[cfg_attr(link_name  = "getpwent_r")]
    pub fn native_getpwent_r(pwd: *mut passwd, buf: *mut ::c_char, buflen: ::c_int) -> *mut passwd;
}

pub unsafe fn getpwent_r(
    pwd: *mut passwd,
    buf: *mut ::c_char,
    buflen: ::size_t,
    result: *mut *mut passwd,
) -> ::c_int {
    let res = native_getpwent_r(passwd, buf, buflen.try_into().unwrap());
    if res.is_null() {
        -1
    } else {
        *result = res;
        0
    }
}

This would go in the src/unix/solarish/compat.rs file, alongside the handful of similar routines that are present there.

We can and likely eventually will figure out a path for reworking these routines in illumos, but it will take time and coordination. Even when that lands it will be months or years before users have updated all their machines past LTS releases of various distributions to get the new symbol available.

SteveLauC · 2022-10-02T01:47:16Z

@jclulow's walkaround looks good and I just added it. Any opinion on this?

(friendly ping @JohnTitor)

jclulow

Thanks for taking a swing at this! I have put a few thoughts in comments.

src/unix/solarish/compat.rs

jclulow · 2022-10-02T05:49:42Z

src/unix/solarish/mod.rs

-        buflen: ::size_t,
-        result: *mut *mut passwd,
-    ) -> ::c_int;
+    pub fn native_getpwent_r(pwd: *mut passwd, buf: *mut ::c_char, buflen: ::c_int) -> *mut passwd;


It's not clear to me if native_getpwent_r() and native_getgrent_r() should be pub or perhaps rather pub(crate) -- or maybe no access qualifier is necessary because they're only used from a child module? If they're pub it would seem that we'd have to keep exposing them forever, because removing them would then break libc crate backwards compatibility.

I removed the pub qualifier since these two functions are only used inside this child module

JohnTitor

If this doesn't introduce any breaking change (i.e. ABI change, MSRV breaking, liking change), I'm fine to accept, has anyone confirmed that?

src/unix/solarish/compat.rs

SteveLauC · 2022-10-03T03:35:33Z

I just squashed and rebased, could I trouble you guys for another review:)

SteveLauC · 2022-10-03T07:13:17Z

API:

Current APIs are the same as the original ones, so no API break here:

MSRV:

IMHO, no MSRV break occurs. Associated constants like ::c_int::MAX require 1.43.0 or above, so I replace it with c_int::max_value(). This function is marked as deprecated, so I am not sure if this is a good approach.

About that `native_getpwent_r()` in `mod.rs`

In the current implementation, this native fn is only visible to that child module. When the desired definition introduced in compat.rs is submitted to the upstream kernel, this fn can be easily replaced with the new one without breaking any APIs.

fn native_getpwent_r(pwd: *mut passwd, buf: *mut ::c_char, buflen: ::c_int) -> *mut passwd;

ctrlcctrlv · 2022-10-03T15:26:20Z

Thanks a lot @jclulow for helping out. As I said in IRC, I think your solution offers the best of my proposed solution and the solution @SteveLauC originally proposed.

JohnTitor · 2022-10-03T22:26:10Z

LGTM, thanks for clarifying!
@jclulow I'm not sure if you approved this or not, could you approve explicitly?

Meanwhile, let's see if CI agrees, @bors try

bors · 2022-10-03T22:26:19Z

⌛ Trying commit 8a914ca with merge 54eb80f...

…sh-os, r=<try> fix wrong definitions of getpwent_r and getgrent_r on solarish os Closes #2908 * [man page for `getpwent_r`](https://illumos.org/man/3C/getpwnam) * [man page for `getgrent_r`](https://illumos.org/man/3C/getgrnam) You may find the definitions for `getpwnam_r/getpwuid_r/getgrnam_r/getgruid_r` exposed by `libc` are also wrong: ```c struct passwd *getpwnam_r(const char *name, struct passwd *pwd, char *buffer, int buflen); ``` ```rust pub fn getpwnam_r( name: *const ::c_char, pwd: *mut passwd, buf: *mut ::c_cha buflen: ::size_t, result: *mut *mut passwd, ) -> ::c_int; ``` But actually they are **correct** as there are the POSIX-conforming definitions (see `Standard conforming` section of above man pages): ``` Standard conforming cc [ flag...] file... -D_POSIX_PTHREAD_SEMANTICS [ library... ] int getpwnam_r(const char *name, struct passwd *pwd, char *buffer, size_t bufsize, struct passwd **result); int getpwuid_r(uid_t uid, struct passwd *pwd, char *buffer, size_t bufsize, struct passwd **result); ``` `getpwent_r/getgrent_r` don't get lucky, they do not have the POSIX-conforming alternatives. To double check this, I searched its [source code](https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/gen/getpwnam_r.c): ```shell $ rg "__posix_getpwnam_r" port/mapfile-vers 1582: __posix_getpwnam_r; port/gen/getpwnam_r.c 152:__posix_getpwnam_r(const char *name, struct passwd *pwd, char *buffer, $ rg "__posix_getpwent_r" $ ```

bors · 2022-10-03T23:00:31Z

💔 Test failed - checks-actions

SteveLauC · 2022-10-03T23:24:24Z

CI Build Channels macOS (1.13.0) and Docker Linux Tier2 (sparc64-unknown-linux-gnu) failed, seems unrelated to this pr

JohnTitor · 2022-10-04T12:13:16Z

CI issue has been fixed, @bors try

…sh-os, r=<try> fix wrong definitions of getpwent_r and getgrent_r on solarish os Closes #2908 * [man page for `getpwent_r`](https://illumos.org/man/3C/getpwnam) * [man page for `getgrent_r`](https://illumos.org/man/3C/getgrnam) You may find the definitions for `getpwnam_r/getpwuid_r/getgrnam_r/getgruid_r` exposed by `libc` are also wrong: ```c struct passwd *getpwnam_r(const char *name, struct passwd *pwd, char *buffer, int buflen); ``` ```rust pub fn getpwnam_r( name: *const ::c_char, pwd: *mut passwd, buf: *mut ::c_cha buflen: ::size_t, result: *mut *mut passwd, ) -> ::c_int; ``` But actually they are **correct** as there are the POSIX-conforming definitions (see `Standard conforming` section of above man pages): ``` Standard conforming cc [ flag...] file... -D_POSIX_PTHREAD_SEMANTICS [ library... ] int getpwnam_r(const char *name, struct passwd *pwd, char *buffer, size_t bufsize, struct passwd **result); int getpwuid_r(uid_t uid, struct passwd *pwd, char *buffer, size_t bufsize, struct passwd **result); ``` `getpwent_r/getgrent_r` don't get lucky, they do not have the POSIX-conforming alternatives. To double check this, I searched its [source code](https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/gen/getpwnam_r.c): ```shell $ rg "__posix_getpwnam_r" port/mapfile-vers 1582: __posix_getpwnam_r; port/gen/getpwnam_r.c 152:__posix_getpwnam_r(const char *name, struct passwd *pwd, char *buffer, $ rg "__posix_getpwent_r" $ ```

bors · 2022-10-04T12:13:23Z

⌛ Trying commit 8a914ca with merge 9d82dce...

bors · 2022-10-04T13:17:13Z

☀️ Try build successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Build commit: 9d82dce (9d82dce8d9ef27620d29e4ea6a0c8e333a7087a7)

JohnTitor · 2022-11-03T09:19:51Z

Since this PR gets no response for a month, I'd like to go ahead assuming @jclulow overall agreed with the change (if you have any objections/concerns, let us know!).
@bors r+

bors · 2022-11-03T09:19:54Z

📌 Commit 8a914ca has been approved by JohnTitor

It is now in the queue for this repository.

bors · 2022-11-03T09:20:00Z

⌛ Testing commit 8a914ca with merge 1ca1c41...

bors · 2022-11-03T10:25:56Z

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 1ca1c41 to master...

rust-highfive assigned Amanieu Sep 18, 2022

rust-highfive added the S-waiting-on-review label Sep 18, 2022

SteveLauC force-pushed the Wrong-getpwent_r-definition-on-solarish-os branch 2 times, most recently from 41a4306 to 85451c6 Compare September 18, 2022 03:45

SteveLauC closed this Sep 18, 2022

SteveLauC reopened this Sep 18, 2022

jclulow reviewed Oct 2, 2022

View reviewed changes

JohnTitor reviewed Oct 2, 2022

View reviewed changes

src/unix/solarish/compat.rs Outdated Show resolved Hide resolved

jclulow reviewed Oct 3, 2022

View reviewed changes

src/unix/solarish/compat.rs Outdated Show resolved Hide resolved

jclulow reviewed Oct 3, 2022

View reviewed changes

src/unix/solarish/compat.rs Outdated Show resolved Hide resolved

SteveLauC force-pushed the Wrong-getpwent_r-definition-on-solarish-os branch 2 times, most recently from 3df8435 to 265b2dd Compare October 3, 2022 03:34

SteveLauC requested review from jclulow and JohnTitor and removed request for jclulow October 3, 2022 03:35

SteveLauC force-pushed the Wrong-getpwent_r-definition-on-solarish-os branch from a73b12c to 2238af3 Compare October 3, 2022 09:43

adopt the get[pw/gr]ent_r def used by FreeBSD

8a914ca

SteveLauC force-pushed the Wrong-getpwent_r-definition-on-solarish-os branch from 2238af3 to 8a914ca Compare October 3, 2022 09:47

JohnTitor approved these changes Oct 3, 2022

View reviewed changes

bors added S-waiting-on-bors and removed S-waiting-on-review labels Nov 3, 2022

bors merged commit 1ca1c41 into rust-lang:master Nov 3, 2022

SteveLauC deleted the Wrong-getpwent_r-definition-on-solarish-os branch November 3, 2022 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix wrong definitions of getpwent_r and getgrent_r on solarish os #2914

fix wrong definitions of getpwent_r and getgrent_r on solarish os #2914

SteveLauC commented Sep 18, 2022

rust-highfive commented Sep 18, 2022

rust-highfive commented Sep 18, 2022

JohnTitor commented Sep 19, 2022

ctrlcctrlv commented Sep 19, 2022

SteveLauC commented Sep 21, 2022

jclulow commented Sep 21, 2022

JohnTitor commented Sep 21, 2022 •

edited

ctrlcctrlv commented Sep 23, 2022

ctrlcctrlv commented Oct 1, 2022

SteveLauC commented Oct 1, 2022

jclulow commented Oct 1, 2022

SteveLauC commented Oct 2, 2022

jclulow left a comment

jclulow Oct 2, 2022

SteveLauC Oct 2, 2022

JohnTitor left a comment

SteveLauC commented Oct 3, 2022

SteveLauC commented Oct 3, 2022 •

edited

ctrlcctrlv commented Oct 3, 2022

JohnTitor commented Oct 3, 2022

bors commented Oct 3, 2022

bors commented Oct 3, 2022

SteveLauC commented Oct 3, 2022

JohnTitor commented Oct 4, 2022

bors commented Oct 4, 2022

bors commented Oct 4, 2022

JohnTitor commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 3, 2022

fix wrong definitions of getpwent_r and getgrent_r on solarish os #2914

fix wrong definitions of getpwent_r and getgrent_r on solarish os #2914

Conversation

SteveLauC commented Sep 18, 2022

rust-highfive commented Sep 18, 2022

rust-highfive commented Sep 18, 2022

JohnTitor commented Sep 19, 2022

ctrlcctrlv commented Sep 19, 2022

SteveLauC commented Sep 21, 2022

jclulow commented Sep 21, 2022

JohnTitor commented Sep 21, 2022 • edited

ctrlcctrlv commented Sep 23, 2022

ctrlcctrlv commented Oct 1, 2022

SteveLauC commented Oct 1, 2022

jclulow commented Oct 1, 2022

SteveLauC commented Oct 2, 2022

jclulow left a comment

Choose a reason for hiding this comment

jclulow Oct 2, 2022

Choose a reason for hiding this comment

SteveLauC Oct 2, 2022

Choose a reason for hiding this comment

JohnTitor left a comment

Choose a reason for hiding this comment

SteveLauC commented Oct 3, 2022

SteveLauC commented Oct 3, 2022 • edited

API:

MSRV:

About that native_getpwent_r() in mod.rs

ctrlcctrlv commented Oct 3, 2022

JohnTitor commented Oct 3, 2022

bors commented Oct 3, 2022

bors commented Oct 3, 2022

SteveLauC commented Oct 3, 2022

JohnTitor commented Oct 4, 2022

bors commented Oct 4, 2022

bors commented Oct 4, 2022

JohnTitor commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 3, 2022

JohnTitor commented Sep 21, 2022 •

edited

SteveLauC commented Oct 3, 2022 •

edited

About that `native_getpwent_r()` in `mod.rs`