mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] fix: Assign default value to mntent when linebuf is too small
@ 2021-10-12  2:36 Kaihang Zhang
  2021-10-12  3:24 ` (GalaxyMaster)
  0 siblings, 1 reply; 5+ messages in thread
From: Kaihang Zhang @ 2021-10-12  2:36 UTC (permalink / raw)
  To: musl, 2010267516; +Cc: Kaihang Zhang

Function getmntent_r in source misc/mntent.c will do what glibc users
expect. The rest of the line will be discarded when can not be read
into linebuf, and the fields of struct mntent will be assigned to empty
string or zero when can not be found in linebuf, instead of setting
errno to ERANGE and exiting.
---
 src/misc/mntent.c | 54 +++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/src/misc/mntent.c b/src/misc/mntent.c
index eabb8200..007c0b8d 100644
--- a/src/misc/mntent.c
+++ b/src/misc/mntent.c
@@ -21,12 +21,12 @@ int endmntent(FILE *f)
 
 struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int buflen)
 {
-	int cnt, n[8], use_internal = (linebuf == SENTINEL);
-
-	mnt->mnt_freq = 0;
-	mnt->mnt_passno = 0;
+	int use_internal = (linebuf == SENTINEL);
+	char *sub;
 
 	do {
+		char *end_ptr;
+
 		if (use_internal) {
 			getline(&internal_buf, &internal_bufsize, f);
 			linebuf = internal_buf;
@@ -34,25 +34,37 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
 			fgets(linebuf, buflen, f);
 		}
 		if (feof(f) || ferror(f)) return 0;
-		if (!strchr(linebuf, '\n')) {
+
+		end_ptr = strchr(linebuf, '\n');
+		if (end_ptr != NULL) {
+			while ((end_ptr[-1] == ' ' || end_ptr[-1] == '\t') && end_ptr != linebuf) end_ptr--;
+			*end_ptr = '\0';
+		} else {
 			fscanf(f, "%*[^\n]%*[\n]");
-			errno = ERANGE;
-			return 0;
 		}
-		cnt = sscanf(linebuf, " %n%*s%n %n%*s%n %n%*s%n %n%*s%n %d %d",
-			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
-			&mnt->mnt_freq, &mnt->mnt_passno);
-	} while (cnt < 2 || linebuf[n[0]] == '#');
-
-	linebuf[n[1]] = 0;
-	linebuf[n[3]] = 0;
-	linebuf[n[5]] = 0;
-	linebuf[n[7]] = 0;
-
-	mnt->mnt_fsname = linebuf+n[0];
-	mnt->mnt_dir = linebuf+n[2];
-	mnt->mnt_type = linebuf+n[4];
-	mnt->mnt_opts = linebuf+n[6];
+
+		linebuf += strspn(linebuf, " \t");
+	} while (linebuf[0] == '\0' || linebuf[0] == '#');
+
+	mnt->mnt_fsname = strsep(&linebuf, " \t");
+
+	if (linebuf) linebuf += strspn(linebuf, " \t");
+	sub = strsep(&linebuf, " \t");
+	mnt->mnt_dir = sub ? sub : (char *) "";
+
+	if (linebuf) linebuf += strspn(linebuf, " \t");
+	sub = strsep (&linebuf, " \t");
+	mnt->mnt_type = sub ? sub : (char *) "";
+
+	if (linebuf) linebuf += strspn(linebuf, " \t");
+	sub = strsep(&linebuf, " \t");
+	mnt->mnt_opts = sub ? sub : (char *) "";
+
+	switch (linebuf ? sscanf(linebuf, " %d %d", &mnt->mnt_freq, &mnt->mnt_passno) : 0) {
+	case 0: mnt->mnt_freq = 0;
+	case 1: mnt->mnt_passno = 0;
+	case 2: break;
+	}
 
 	return mnt;
 }
-- 
2.25.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [musl] [PATCH] fix: Assign default value to mntent when linebuf is too small
  2021-10-12  2:36 [musl] [PATCH] fix: Assign default value to mntent when linebuf is too small Kaihang Zhang
@ 2021-10-12  3:24 ` (GalaxyMaster)
  2021-10-12 12:59   ` Rich Felker
  0 siblings, 1 reply; 5+ messages in thread
From: (GalaxyMaster) @ 2021-10-12  3:24 UTC (permalink / raw)
  To: musl; +Cc: 2010267516, Kaihang Zhang

Kaihang,

On Mon, Oct 11, 2021 at 10:36:43PM -0400, Kaihang Zhang wrote:
> Function getmntent_r in source misc/mntent.c will do what glibc users
> expect. The rest of the line will be discarded when can not be read
> into linebuf, and the fields of struct mntent will be assigned to empty
> string or zero when can not be found in linebuf, instead of setting
> errno to ERANGE and exiting.

Although this patch is on a similar topic as mine (changing the behaviour of
get*ent() funnctions), I think the change you are describing is considerable.

I would expect a function such as getmntent_r() which takes a user provided
buffer to fail and set ERANGE if the provided buffer is not enough to hold
the line.  This gives the developer an opportunity to recover, e.g. to
re-allocate a bigger buffer and try again.

In your proposal, I see two issues:

1. There is no feedback to the developer, so they have no idea whether the
   information they've got from the function was truncated or not (and what
   good does a truncated mnt line bring?);
2. There is no opportunity for the developer to realise a mistake they made
   by supplying too small buffer, hence there is no chance of recovering
   from it.

It is just my opinion and I would love to see other comments, since I have
not stumbled upon your use case yet and am not authoritative on this topic.

-- 
(GM)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [musl] [PATCH] fix: Assign default value to mntent when linebuf is too small
  2021-10-12  3:24 ` (GalaxyMaster)
@ 2021-10-12 12:59   ` Rich Felker
  2021-10-15  7:19     ` Kaihang Zhang
  2021-10-27 14:34     ` [musl] Changes to getmntent Alyssa Ross
  0 siblings, 2 replies; 5+ messages in thread
From: Rich Felker @ 2021-10-12 12:59 UTC (permalink / raw)
  To: (GalaxyMaster); +Cc: musl, 2010267516, Kaihang Zhang

On Tue, Oct 12, 2021 at 03:24:07AM +0000, (GalaxyMaster) wrote:
> Kaihang,
> 
> On Mon, Oct 11, 2021 at 10:36:43PM -0400, Kaihang Zhang wrote:
> > Function getmntent_r in source misc/mntent.c will do what glibc users
> > expect. The rest of the line will be discarded when can not be read
> > into linebuf, and the fields of struct mntent will be assigned to empty
> > string or zero when can not be found in linebuf, instead of setting
> > errno to ERANGE and exiting.
> 
> Although this patch is on a similar topic as mine (changing the behaviour of
> get*ent() funnctions), I think the change you are describing is considerable.
> 
> I would expect a function such as getmntent_r() which takes a user provided
> buffer to fail and set ERANGE if the provided buffer is not enough to hold
> the line.  This gives the developer an opportunity to recover, e.g. to
> re-allocate a bigger buffer and try again.
> 
> In your proposal, I see two issues:
> 
> 1. There is no feedback to the developer, so they have no idea whether the
>    information they've got from the function was truncated or not (and what
>    good does a truncated mnt line bring?);
> 2. There is no opportunity for the developer to realise a mistake they made
>    by supplying too small buffer, hence there is no chance of recovering
>    from it.
> 
> It is just my opinion and I would love to see other comments, since I have
> not stumbled upon your use case yet and am not authoritative on this topic.

Yes. There are actually several conflicting proposed changes to this
(frustratingly underspecified, and with bad behavior by glibc)
function, and so far there's been little engagement between different
people who want it changed/fixed to resolve the differences. I would
really like to see from at least one party who wants it changed a
summary of what the differences are between the different proposals,
what musl is doing now, and what glibc is doing now, and
justifications for why their preferred one is okay (including
capability for applications to recover and not silently use wrong data
-- "glibc lets them silently use wrong data" isn't a justification).

Rich

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [musl] [PATCH] fix: Assign default value to mntent when linebuf is too small
  2021-10-12 12:59   ` Rich Felker
@ 2021-10-15  7:19     ` Kaihang Zhang
  2021-10-27 14:34     ` [musl] Changes to getmntent Alyssa Ross
  1 sibling, 0 replies; 5+ messages in thread
From: Kaihang Zhang @ 2021-10-15  7:19 UTC (permalink / raw)
  To: Rich Felker; +Cc: (GalaxyMaster), musl, care

On Tue, Oct 12, 2021 at 8:59 PM Rich Felker <dalias@libc.org> wrote:
>
> On Tue, Oct 12, 2021 at 03:24:07AM +0000, (GalaxyMaster) wrote:
> > Kaihang,
> >
> > On Mon, Oct 11, 2021 at 10:36:43PM -0400, Kaihang Zhang wrote:
> > > Function getmntent_r in source misc/mntent.c will do what glibc users
> > > expect. The rest of the line will be discarded when can not be read
> > > into linebuf, and the fields of struct mntent will be assigned to empty
> > > string or zero when can not be found in linebuf, instead of setting
> > > errno to ERANGE and exiting.
> >
> > Although this patch is on a similar topic as mine (changing the behaviour of
> > get*ent() funnctions), I think the change you are describing is considerable.
> >
> > I would expect a function such as getmntent_r() which takes a user provided
> > buffer to fail and set ERANGE if the provided buffer is not enough to hold
> > the line.  This gives the developer an opportunity to recover, e.g. to
> > re-allocate a bigger buffer and try again.
> >
> > In your proposal, I see two issues:
> >
> > 1. There is no feedback to the developer, so they have no idea whether the
> >    information they've got from the function was truncated or not (and what
> >    good does a truncated mnt line bring?);
> > 2. There is no opportunity for the developer to realise a mistake they made
> >    by supplying too small buffer, hence there is no chance of recovering
> >    from it.
> >
> > It is just my opinion and I would love to see other comments, since I have
> > not stumbled upon your use case yet and am not authoritative on this topic.
>
> Yes. There are actually several conflicting proposed changes to this
> (frustratingly underspecified, and with bad behavior by glibc)
> function, and so far there's been little engagement between different
> people who want it changed/fixed to resolve the differences. I would
> really like to see from at least one party who wants it changed a
> summary of what the differences are between the different proposals,
> what musl is doing now, and what glibc is doing now, and
> justifications for why their preferred one is okay (including
> capability for applications to recover and not silently use wrong data
> -- "glibc lets them silently use wrong data" isn't a justification).
>
> Rich

Give an example to better describe it. The contents of /proc/mounts
are like below,
the field 'opts' of overlay can be even more longer.

> overlay / overlay rw,seclabel,relatime,lowerdir=/var/lib/docker/overlay2/l/6WSJ5KZAYZO55RQRWFQZNDGPWK:/var/lib/docker/overlay2/l/YS7ZKHUX3JB3F4SQIJYXXNIYTV:/var/lib/docker/overlay2/l/OWUPK4QNZP7ZOZKERGMTJQW55T:/var/lib/docker/overlay2/l/LL7NIEVB5NMPAEJK6SUCXJYRGK,upperdir=/var/lib/docker/overlay2/2012ae841481001d5eea099c6e84319f88c5079d44eb6a2402397858dd3d22e2/diff,workdir=/var/lib/docker/overlay2/2012ae841481001d5eea099c6e84319f88c5079d44eb6a2402397858dd3d22e2/work 0 0
> proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0
> cgroup /sys/fs/cgroup/systemd cgroup rw,seclabel,nosuid,nodev,noexec,relatime,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd 0 0

1. The behavior of getmntent_r in glibc.
 If the buffer is too samll for the line 'overlay', the line will be truncated.
 Glibc uses wrong data for this line. But i only care about cgroup info,
 and i am sure that the buffer capacity is sufficient for cgroup. The mntent
 for cgroup is exactly what i want. There is no need to retry use a
bigger buffer.

 However, as galaxy wrote:
 >    There is no opportunity for the developer to realise a mistake they made
 >    by supplying too small buffer, hence there is no chance of recovering
 >    from it.
 The user gets the wrong data and doesn't realize it.

2. The behavior of getmntent_r in musl libc.
  If the buffer is too small for the line 'overlay', it will set errno
to ERANGE and
  then exit. And the user can retry with a bigger buffer.

  However, as ericonr ever said:
  > The current interface doesn't allow trying again to read the too-long entry,
  > since it will just have moved to the next one.
  The too-long entry data will be lost. Maybe fseek(f, -fgets_result, SEEK_CUR)
  could be used to rewind until the start of the line, but the
function would just
  loop eternally with too short a buffer, for example.
  > The overlay entry has 2000 bytes, the size of buffer is 256 bytes,
if errno is ERANGE
  > i will retry use buffer size 512, 768, 1024, 1280, 1536, 1792,
2048... I have to try at least
  > 8 times to get the cgroup info ! :(

In my opinion, it's ok to truncate the too-long entry (musl libc will
even lose it ! ) instead exiting.
And the errno will be set to ERANGE in order to let uesr realise this
mistake, but as i know
the user can only retry from the beginning of the file, that bothers
me somehow. It‘s up to the user
to decide to retry or not. If the user doesn't care about the too-long
entry or the discarded contents of it,
there is no need to retry. Othrewise, retry from the beginning of the file.

Kaihang

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [musl] Changes to getmntent
  2021-10-12 12:59   ` Rich Felker
  2021-10-15  7:19     ` Kaihang Zhang
@ 2021-10-27 14:34     ` Alyssa Ross
  1 sibling, 0 replies; 5+ messages in thread
From: Alyssa Ross @ 2021-10-27 14:34 UTC (permalink / raw)
  To: Rich Felker, (GalaxyMaster); +Cc: musl, 2010267516, Kaihang Zhang

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

Rich Felker <dalias@libc.org> writes:

> Yes. There are actually several conflicting proposed changes to this
> (frustratingly underspecified, and with bad behavior by glibc)
> function, and so far there's been little engagement between different
> people who want it changed/fixed to resolve the differences. I would
> really like to see from at least one party who wants it changed a
> summary of what the differences are between the different proposals,
> what musl is doing now, and what glibc is doing now, and
> justifications for why their preferred one is okay (including
> capability for applications to recover and not silently use wrong data
> -- "glibc lets them silently use wrong data" isn't a justification).
>
> Rich

Hi, I'm somebody who has proposed changes to getmntent recently, so here
I am specifying what I'd like it to do:

 • Lines starting with "#" should be skipped.
 • Empty lines should be skipped.
 • The fifth and sixth fields should be optional.

All of these changes are things that glibc does today, and musl does
not.

Hopefully it's pretty obvious why the first two are good ideas.  As for
the third, I think it's a change to be made because the last two fields
(dump frequency, and pass number on parallel fsck) are obscure and not
required to mount the filesystem, and because fstab(5) says they're
optional.

The approach I've been taking so far is to send a series containing
interspersed patches to libc-test and musl, with the idea that libc-test
could be the place where we specify how genmntent should work.  Perhaps,
to accelerate the process of getting everybody on the same page, it
would be better to concentrate on getting tests into libc-test that
describe the behaviour we think musl should have, and once that's done
begin to look at changing musl's implementation to pass the test.  Would
that be a good way forward?  I'd be happy to kick things off with a
patch to libc-test that tests basic functionality and all the behaviour
changes I'd like to have made, if Rich is happy to go that way.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-10-27 14:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12  2:36 [musl] [PATCH] fix: Assign default value to mntent when linebuf is too small Kaihang Zhang
2021-10-12  3:24 ` (GalaxyMaster)
2021-10-12 12:59   ` Rich Felker
2021-10-15  7:19     ` Kaihang Zhang
2021-10-27 14:34     ` [musl] Changes to getmntent Alyssa Ross

Code repositories for project(s) associated with this inbox:

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).