Re: [PATCH] linedit, deluser: use POSIX getpwent instead of getpwent_r

Re: [PATCH] linedit, deluser: use POSIX getpwent instead of getpwent_r

[Rich Felker]

On Sat, Feb 07, 2015 at 03:14:10PM +0100, Denys Vlasenko wrote:
> On Sat, Feb 7, 2015 at 2:32 AM, Rich Felker <dalias@libc.org> wrote:
> >> > the _r functions are for thread-safe
> >> > versions of their corresponding legacy functions, but getpwent_r has
> >> > inherent global state -- the iterator. Whoever made it just wasn't
> >> > thinking. To make a correct interface like this the caller would need
> >> > to have an iterator object to pass to the function, but I can't see
> >> > much merit in inventing a new interface for this.
> >>
> >> It doesn't matter that it makes no sense. It's glibc compat.
> >> If you want more programs rather than less to build
> >> against musl, you have to strive to be glibc-compatible where practical.
> >
> > So far we haven't hit anything wanting to use it except busybox. I'm
> > all for compatibility, but it doesn't look easy to provide without
> > ugly code duplication or similar. We're in the middle of reworking
> > some of this code anyway to add alternate backend support, so I just
> > asked about how easy it would be to get getpwent_r too, but we didn't
> > see any obvious clean ways to do it. This is basically a consequence
> > of the way musl uses a dynamic buffer for the strings (line from the
> > file) so as not to impose an arbitrary line limit,
> 
> How about this implementation?
> 
> int getpwent_r(struct passwd *pwbuf, char *buf, size_t buflen, struct
> passwd **pwbufp)
> {
>         char *line=0;
>         size_t size=0;
>         if (!f) f = fopen("/etc/passwd", "rbe");
>         if (!f) return 0;
>         *pwbufp = __getpwent_a(f, pwbuf, &line, &size);
>         if (!*pwbufp)
>                 return 0; /* success (eof) */
>         if (size < buflen)
>                 strcpy(buf, line);
>         free(line);
>         if (size < buflen)
>                 return 0; /* success */
>         *pwbufp = 0;
>         errno = ERANGE;
>         return ERANGE;
> }

Now *pwbuf contains a bunch of invalid ("dangling") pointers. They
need to be fixed-up the same way getpw_r.c is doing it right now,
which is where the code duplication comes in. It's not huge but it's
ugly having the same logic repeated two places (and again for groups
once you do groups too).

Rich

Re: Re: [PATCH] linedit, deluser: use POSIX getpwent instead of getpwent_r

[M Farkas-Dyck]

On 07/02/2015, Rich Felker <dalias@libc.org> wrote:
> On Sat, Feb 07, 2015 at 03:14:10PM +0100, Denys Vlasenko wrote:
>> On Sat, Feb 7, 2015 at 2:32 AM, Rich Felker <dalias@libc.org> wrote:
>> >> > the _r functions are for thread-safe
>> >> > versions of their corresponding legacy functions, but getpwent_r has
>> >> > inherent global state -- the iterator. Whoever made it just wasn't
>> >> > thinking. To make a correct interface like this the caller would
>> >> > need
>> >> > to have an iterator object to pass to the function, but I can't see
>> >> > much merit in inventing a new interface for this.

buf may contain arbitrary data, yes? If so we could store the iterator there.

Re: Re: [PATCH] linedit, deluser: use POSIX getpwent instead of getpwent_r

[Rich Felker]

On Mon, Feb 09, 2015 at 01:05:07PM -0500, M Farkas-Dyck wrote:
> On 07/02/2015, Rich Felker <dalias@libc.org> wrote:
> > On Sat, Feb 07, 2015 at 03:14:10PM +0100, Denys Vlasenko wrote:
> >> On Sat, Feb 7, 2015 at 2:32 AM, Rich Felker <dalias@libc.org> wrote:
> >> >> > the _r functions are for thread-safe
> >> >> > versions of their corresponding legacy functions, but getpwent_r has
> >> >> > inherent global state -- the iterator. Whoever made it just wasn't
> >> >> > thinking. To make a correct interface like this the caller would
> >> >> > need
> >> >> > to have an iterator object to pass to the function, but I can't see
> >> >> > much merit in inventing a new interface for this.
> 
> buf may contain arbitrary data, yes? If so we could store the iterator there.

No. On entry the buffer must be assumed to be uninitialized. The
caller could be passing a completely new buffer each time, and even if
not, on the first call the buffer is going to contain junk. The
contract of getpwent_r, at least on glibc and compatible systems,
seems to be that it uses the same iterator as getpwent. But since it
turns out there are multiple incompatible historical functions names
getpwent_r that all behave differently, this proposal has basically
been shelved anyway. Busybox has dropped its use of getpwent_r.

Rich