<shadow.h> function: fgetspent_r

<shadow.h> function: fgetspent_r

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 754 bytes --]

Hi muslers,

fgetspent_r[1] is a re-entrant version of fgetspent which stores all
strings in a caller-provided buffer to ensure that the memory is owned
by the caller instead of by the system.

It is present in Solaris 9[2] and higher, and glibc[3] Linux.  It is
used by AccountsService[4].

Is it possible to add this API to musl?  I could try to write it, if so.

Best,
--arw


[1]: https://docs.oracle.com/cd/E88353_01/html/E37843/getspent-r-3c.html

[2]: https://docs.oracle.com/cd/E19683-01/816-5214/6mbcfdl0v/index.html

[3]: https://linux.die.net/man/3/getspnam_r

[4]: https://cgit.freedesktop.org/accountsservice/commit/?id=14ca4245


-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: <shadow.h> function: fgetspent_r

[Rich Felker]

On Wed, Jan 16, 2019 at 01:21:39PM -0600, A. Wilcox wrote:
> Hi muslers,
> 
> fgetspent_r[1] is a re-entrant version of fgetspent which stores all
> strings in a caller-provided buffer to ensure that the memory is owned
> by the caller instead of by the system.
> 
> It is present in Solaris 9[2] and higher, and glibc[3] Linux.  It is
> used by AccountsService[4].
> 
> Is it possible to add this API to musl?  I could try to write it, if so.
> 
> Best,
> --arw
> 
> 
> [1]: https://docs.oracle.com/cd/E88353_01/html/E37843/getspent-r-3c.html
> 
> [2]: https://docs.oracle.com/cd/E19683-01/816-5214/6mbcfdl0v/index.html
> 
> [3]: https://linux.die.net/man/3/getspnam_r
> 
> [4]: https://cgit.freedesktop.org/accountsservice/commit/?id=14ca4245

I don't see any good reason why it couldn't be added, but it doesn't
look like a direct refactoring of the existing function since it uses
the messy char[] buffer idiom like a bunch of other _r functions in
this family. I'm also not sure what should happen if the next entry
does not fit in the buffer. Should it discard the rest of the line and
move on (not retryable) or attempt to seek back?

Rich

Re: <shadow.h> function: fgetspent_r

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 2513 bytes --]

On 01/16/19 14:50, Rich Felker wrote:
> On Wed, Jan 16, 2019 at 01:21:39PM -0600, A. Wilcox wrote:
>> Hi muslers,
>>
>> fgetspent_r[1] is a re-entrant version of fgetspent which stores all
>> strings in a caller-provided buffer to ensure that the memory is owned
>> by the caller instead of by the system.
>>
>> It is present in Solaris 9[2] and higher, and glibc[3] Linux.  It is
>> used by AccountsService[4].
>>
>> Is it possible to add this API to musl?  I could try to write it, if so.
>>
>> Best,
>> --arw
>>
>>
>> [1]: https://docs.oracle.com/cd/E88353_01/html/E37843/getspent-r-3c.html
>>
>> [2]: https://docs.oracle.com/cd/E19683-01/816-5214/6mbcfdl0v/index.html
>>
>> [3]: https://linux.die.net/man/3/getspnam_r
>>
>> [4]: https://cgit.freedesktop.org/accountsservice/commit/?id=14ca4245
> 
> I don't see any good reason why it couldn't be added, but it doesn't
> look like a direct refactoring of the existing function since it uses
> the messy char[] buffer idiom like a bunch of other _r functions in
> this family.


What do you mean by "messy char[] buffer idiom"?  The buffer that is
meant to contain the strings is passed to the function (char *buf),
*not* returned by it.


> I'm also not sure what should happen if the next entry
> does not fit in the buffer. Should it discard the rest of the line and
> move on (not retryable) or attempt to seek back?


The Solaris docs specify:

fgetspent_r() [ ... ] return a pointer to a struct spwd if they
successfully enumerate an entry; otherwise they return NULL, indicating
the end of the enumeration.

The reentrant functions getspnam_r(), getspent_r(), and fgetspent_r()
will return NULL and set errno to ERANGE if the length of the buffer
supplied by caller is not large enough to store the result.


The glibc docs specify:

A pointer to the result (in case of success) or NULL (in case no entry
was found or an error occurred) is stored in *spbufp.

The reentrant functions return zero on success. In case of error, an
error number is returned.

ERANGE: Supplied buffer is too small.



It seems like "end of file" and "error" are treated the same.  In any
case, it would appear to me that it's UB to continue iterating once NULL
is returned.  I would personally leave the fp where it is (not rewind)
since all the other *get*ent functions don't rewind on error either.

Best,
--arw


-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: <shadow.h> function: fgetspent_r

[Rich Felker]

On Wed, Jan 16, 2019 at 03:38:06PM -0600, A. Wilcox wrote:
> On 01/16/19 14:50, Rich Felker wrote:
> > On Wed, Jan 16, 2019 at 01:21:39PM -0600, A. Wilcox wrote:
> >> Hi muslers,
> >>
> >> fgetspent_r[1] is a re-entrant version of fgetspent which stores all
> >> strings in a caller-provided buffer to ensure that the memory is owned
> >> by the caller instead of by the system.
> >>
> >> It is present in Solaris 9[2] and higher, and glibc[3] Linux.  It is
> >> used by AccountsService[4].
> >>
> >> Is it possible to add this API to musl?  I could try to write it, if so.
> >>
> >> Best,
> >> --arw
> >>
> >>
> >> [1]: https://docs.oracle.com/cd/E88353_01/html/E37843/getspent-r-3c.html
> >>
> >> [2]: https://docs.oracle.com/cd/E19683-01/816-5214/6mbcfdl0v/index.html
> >>
> >> [3]: https://linux.die.net/man/3/getspnam_r
> >>
> >> [4]: https://cgit.freedesktop.org/accountsservice/commit/?id=14ca4245
> > 
> > I don't see any good reason why it couldn't be added, but it doesn't
> > look like a direct refactoring of the existing function since it uses
> > the messy char[] buffer idiom like a bunch of other _r functions in
> > this family.
> 
> What do you mean by "messy char[] buffer idiom"?  The buffer that is
> meant to contain the strings is passed to the function (char *buf),
> *not* returned by it.

It's also necessarily used to contain the struct itself, despite C not
really allowing this and the buffer not being properly aligned for it
(requiring realignment which is inherently nonportable and not even
possible in something like a memory-safe implementation). A proper API
would have the caller pass both a pointer to the struct to fill and a
pointer to char[] space for strings.

> > I'm also not sure what should happen if the next entry
> > does not fit in the buffer. Should it discard the rest of the line and
> > move on (not retryable) or attempt to seek back?
> 
> The Solaris docs specify:
> 
> fgetspent_r() [ ... ] return a pointer to a struct spwd if they
> successfully enumerate an entry; otherwise they return NULL, indicating
> the end of the enumeration.
> 
> The reentrant functions getspnam_r(), getspent_r(), and fgetspent_r()
> will return NULL and set errno to ERANGE if the length of the buffer
> supplied by caller is not large enough to store the result.
> 
> 
> The glibc docs specify:
> 
> A pointer to the result (in case of success) or NULL (in case no entry
> was found or an error occurred) is stored in *spbufp.
> 
> The reentrant functions return zero on success. In case of error, an
> error number is returned.
> 
> ERANGE: Supplied buffer is too small.
> 
> 
> 
> It seems like "end of file" and "error" are treated the same.  In any
> case, it would appear to me that it's UB to continue iterating once NULL
> is returned.

Well at least has unspecified behavior; it's not necessarily
undefined.

> I would personally leave the fp where it is (not rewind)
> since all the other *get*ent functions don't rewind on error either.

But should it finish consuming the line it's in the middle of?
Otherwise it could wrongly interpret the remainder of the line as a
complete line if called again. Note that the current version of the
non-_r function has that issue if getline OOM's..

Rich

Re: <shadow.h> function: fgetspent_r

[Markus Wichmann]

On Wed, Jan 16, 2019 at 06:44:10PM -0500, Rich Felker wrote:
> On Wed, Jan 16, 2019 at 03:38:06PM -0600, A. Wilcox wrote:
> > What do you mean by "messy char[] buffer idiom"?  The buffer that is
> > meant to contain the strings is passed to the function (char *buf),
> > *not* returned by it.
> 
> It's also necessarily used to contain the struct itself, despite C not
> really allowing this and the buffer not being properly aligned for it
> (requiring realignment which is inherently nonportable and not even
> possible in something like a memory-safe implementation). A proper API
> would have the caller pass both a pointer to the struct to fill and a
> pointer to char[] space for strings.
> 

Erm, no, there is a parameter where you can store the struct itself.
It's the second parameter. You are supposed to set the target of the
fifth parameter equal to the second on success, else to NULL.

Working on it, but that is even more messy. What to do on EOF? The only
thing I can think of is return 0, but set *spbufp to 0 (wait, is that
the only reason return value and return pointer are split? Dear lord...)

What to do on "buffer too small"? Seek the file back to where it was?

What to return on format error?

What to do on "buffer too large"? I wanted to use fgets() to read the
next line, but the size parameter of fgets() is an int, so the size
parameter to fgetspent_r() can't exceed INT_MAX.

> > I would personally leave the fp where it is (not rewind)
> > since all the other *get*ent functions don't rewind on error either.
> 
> But should it finish consuming the line it's in the middle of?
> Otherwise it could wrongly interpret the remainder of the line as a
> complete line if called again. Note that the current version of the
> non-_r function has that issue if getline OOM's..
> 
> Rich

Good point, that's a possible problem as well. We could rid ourselves of
these problems by declaring a new call after error to be UB. But that's
not QoI...

Ciao,
Markus

Re: <shadow.h> function: fgetspent_r

[Rich Felker]

On Thu, Jan 17, 2019 at 06:31:47AM +0100, Markus Wichmann wrote:
> On Wed, Jan 16, 2019 at 06:44:10PM -0500, Rich Felker wrote:
> > On Wed, Jan 16, 2019 at 03:38:06PM -0600, A. Wilcox wrote:
> > > What do you mean by "messy char[] buffer idiom"?  The buffer that is
> > > meant to contain the strings is passed to the function (char *buf),
> > > *not* returned by it.
> > 
> > It's also necessarily used to contain the struct itself, despite C not
> > really allowing this and the buffer not being properly aligned for it
> > (requiring realignment which is inherently nonportable and not even
> > possible in something like a memory-safe implementation). A proper API
> > would have the caller pass both a pointer to the struct to fill and a
> > pointer to char[] space for strings.
> > 
> 
> Erm, no, there is a parameter where you can store the struct itself.
> It's the second parameter. You are supposed to set the target of the
> fifth parameter equal to the second on success, else to NULL.

Oh, okay, I misread and was thinking it worked like the
gethostbyname_r etc. family.

> Working on it, but that is even more messy. What to do on EOF? The only
> thing I can think of is return 0, but set *spbufp to 0 (wait, is that
> the only reason return value and return pointer are split? Dear lord...)

I think so. That kind of mess is common in these interfaces.

> What to do on "buffer too small"? Seek the file back to where it was?

Indeed that sounds best at first but it depends on the stream being
seekable, making behavior inconsistent depending on whether it's used
with a seekable or nonseekable stream.

> What to return on format error?

No idea.

> What to do on "buffer too large"? I wanted to use fgets() to read the
> next line, but the size parameter of fgets() is an int, so the size
> parameter to fgetspent_r() can't exceed INT_MAX.

You can use fgets with large buffers but have to call it again if the
buffer is filled without hitting a newline. This of course is a pain.

However the right behavior is not to consider the error "buffer too
large" but rather "record too large". Records never need to exceed the
max username length plus the max plausibly-reasonable hash length plus
some space for the numeric fields. The size argument can then just be
clipped to something reasonable, and in this case hitting it is just
an invalid record.

FWIW the current implementation of getspnam_r has a bug in that it
silently ignores this issue if the caller passes a huge size. That
should be fixed too.

> > > I would personally leave the fp where it is (not rewind)
> > > since all the other *get*ent functions don't rewind on error either.
> > 
> > But should it finish consuming the line it's in the middle of?
> > Otherwise it could wrongly interpret the remainder of the line as a
> > complete line if called again. Note that the current version of the
> > non-_r function has that issue if getline OOM's..
> 
> Good point, that's a possible problem as well. We could rid ourselves of
> these problems by declaring a new call after error to be UB. But that's
> not QoI...

Note that getspnam_r already handles this correctly. After getting a
read with no final newline, it goes into a skip mode where it
continues to call fgets and discard input until a newline is hit.

Rich

Re: <shadow.h> function: fgetspent_r

[Markus Wichmann]

Hi all,

so I had a look at the glibc implementation (from v2.24, which I had
lying around), and here are my findings: They don't return on format
error. Instead, they loop.

On buffer too short, they will leave the stream where it is and return
ERANGE. And on end of file, they return ENOENT, which I find bizarre.

Looking at my own implementation, I am not certain if cleaning up a
faulty state is desirable, since non-recoverable situations exist.
fgetspent_r() has the invariant that the file position is pointing to
the start of a line. So, first thought was to use ftell() beforehand and
fseek() on error, to revert to the start of a line. However, that really
only helps with the "buffer too short" case. An illegal line won't
become more acceptable, no matter how many times we reread it.

Rich correctly pointed out that a file might not be seekable. So if
reversing isn't an option, we can only go forward. So I added an fgets()
loop in case the fseek() fails. But fgets() can fail, too. Maybe even
spuriously (what happens if the file is actually an fdopen()ed
nonblocking TCP socket or something?), so that the problem fixes itself.
Then the invariant is still violated and we have no indication of
anything being wrong.

So, since it is impossible to guarantee the invariant, and the user
certainly has no way to check, I think telling the user to close the
file on any error is probably the best choice here (i.e. the choice that
saves me the most work).

Looping on format error sounds attractive as well, but I think we'd want
that behavior to be consistent across the entire src/passwd directory,
right? It would allow slightly broken files to be present in the system
without bricking it. On the other hand, permissivity is usually only a
good thing outside of security contexts.

Thoughts?

Ciao,
Markus

Re: <shadow.h> function: fgetspent_r

[Markus Wichmann]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

Hi all,

so, I wrote a version of fgetspent_r() now. I based it on fgetspent().
For style, I adopted the Linux style -- might need to refactor that.
Returning EILSEQ on format error is a hack, but I found no better code.
As I said, glibc loops on error, but we do that for no other src/passwd
function, so we should either not start now or add that feature to every
other function.

One thing I noticed: If AccountService requires this interfaces, is it
possible that it doesn't support TCB shadow files?

Ciao,
Markus

[-- Attachment #2: 0005-Add-fgetspent_r.patch --]
[-- Type: text/x-diff, Size: 1887 bytes --]

From 3489f9e5ef055c80464252fe640fead8aeb1068e Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Sun, 20 Jan 2019 16:31:34 +0100
Subject: [PATCH 5/5] Add fgetspent_r().

Interface was defined by glibc, and seems to have been adopted by
Solaris. Some freedesktop software appears to require it, and it adds
little bloat.

Added without feature test macros, since no other interface in shadow.h
requires it, even the ones documented to have required it in the past.
---
 include/shadow.h         |  1 +
 src/passwd/fgetspent_r.c | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 src/passwd/fgetspent_r.c

diff --git a/include/shadow.h b/include/shadow.h
index 2b1be413..4edc90db 100644
--- a/include/shadow.h
+++ b/include/shadow.h
@@ -33,6 +33,7 @@ int putspent(const struct spwd *, FILE *);
 
 struct spwd *getspnam(const char *);
 int getspnam_r(const char *, struct spwd *, char *, size_t, struct spwd **);
+int fgetspent_r(FILE *f, struct spwd* sp, char *line, size_t size, struct spwd **spret);
 
 int lckpwdf(void);
 int ulckpwdf(void);
diff --git a/src/passwd/fgetspent_r.c b/src/passwd/fgetspent_r.c
new file mode 100644
index 00000000..643637de
--- /dev/null
+++ b/src/passwd/fgetspent_r.c
@@ -0,0 +1,27 @@
+#include "pwf.h"
+#include <pthread.h>
+#include <limits.h>
+#include <stdio.h>
+
+int fgetspent_r(FILE *f, struct spwd* sp, char *line, size_t size, struct spwd **spret)
+{
+	int res = 0;
+	int cs;
+	*spret = 0;
+	if (size > INT_MAX)
+		size = INT_MAX; //2GB ought to be enough for anyone
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+	if (!fgets(line, size, f))
+		goto out;
+	res = ERANGE;
+	if (line[strlen(line) - 1] != '\n')
+		goto out;
+	res = EILSEQ;
+	if ( __parsespent(line, sp) < 0)
+		goto out;
+	*spret = sp;
+	res = 0;
+out:
+	pthread_setcancelstate(cs, 0);
+	return res;
+}
-- 
2.19.1

Re: <shadow.h> function: fgetspent_r

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 1044 bytes --]

On 01/20/19 09:41, Markus Wichmann wrote:
> Hi all,
> 
> so, I wrote a version of fgetspent_r() now. I based it on fgetspent().
> For style, I adopted the Linux style -- might need to refactor that.
> Returning EILSEQ on format error is a hack, but I found no better code.
> As I said, glibc loops on error, but we do that for no other src/passwd
> function, so we should either not start now or add that feature to every
> other function.
> 
> One thing I noticed: If AccountService requires this interfaces, is it
> possible that it doesn't support TCB shadow files?
> 
> Ciao,
> Markus


It doesn't support TCB shadow files.


> +int fgetspent_r(FILE *f, struct spwd* sp, char *line, size_t size,
struct spwd **spret)

Tiny style nit: I think the * is meant to be kept with 'sp', not 'spwd',
in the second argument.


Thank you so much for this!  I will test this patch out ASAP and report
how well it works.

Best,
--arw

-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: <shadow.h> function: fgetspent_r

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 573 bytes --]

On 01/20/19 09:41, Markus Wichmann wrote:
> +	int res = 0;

This needs to be EIO; 0 means success, and causes AccountsService to
dereference the NULL that it receives when the file is exhausted (no
more lines).

Other than this, basic functionality seems to work.  I need to port
KDE's User Manager from logind to ConsoleKit2 before I can test it
fully, but listing (the part that uses fgetspent_r) gives the correct list.

Thank you again!

Best to you and yours,
--arw

-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: <shadow.h> function: fgetspent_r

[Rich Felker]

On Sun, Jan 20, 2019 at 03:12:59PM -0600, A. Wilcox wrote:
> On 01/20/19 09:41, Markus Wichmann wrote:
> > Hi all,
> > 
> > so, I wrote a version of fgetspent_r() now. I based it on fgetspent().
> > For style, I adopted the Linux style -- might need to refactor that.
> > Returning EILSEQ on format error is a hack, but I found no better code.
> > As I said, glibc loops on error, but we do that for no other src/passwd
> > function, so we should either not start now or add that feature to every
> > other function.
> > 
> > One thing I noticed: If AccountService requires this interfaces, is it
> > possible that it doesn't support TCB shadow files?
> > 
> > Ciao,
> > Markus
> 
> 
> It doesn't support TCB shadow files.

How so? The whole point of this interface is that it reads from a
FILE* provided by the caller, not the system shadow store. The usage
case is writing utilities that access and modify the underlying files
(or possibly tempfile copies thereof).

> > +int fgetspent_r(FILE *f, struct spwd* sp, char *line, size_t size,
> struct spwd **spret)
> 
> Tiny style nit: I think the * is meant to be kept with 'sp', not 'spwd',
> in the second argument.

Yes.

Rich