[musl] [PATCH v2] cuserid: support invocation with a NULL pointer argument

[musl] [PATCH v2] cuserid: support invocation with a NULL pointer argument

[Sören Tempel]

I did not manage to find a copy of IEEE 1003.1-1988 (the last POSIX
version where cuserid was last standardized) the Single UNIX
specification version 2 does state the following though [1]:

	If s is a null pointer, this representation is generated in an
	area that may be static (and thus overwritten by subsequent
	calls to cuserid()), the address of which is returned.

Even though this a legacy function it would therefore be nice for musl
to support usage with a NULL pointer. I ran into this on Alpine Linux
when using cdparanoia [2] which uses cuserid like this and therefore
caused a crash on my system.

[1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/cuserid.html
[2]: https://xiph.org/paranoia/index.html
---
 src/legacy/cuserid.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/legacy/cuserid.c b/src/legacy/cuserid.c
index 4e78798d..fd7832e4 100644
--- a/src/legacy/cuserid.c
+++ b/src/legacy/cuserid.c
@@ -5,10 +5,12 @@
 
 char *cuserid(char *buf)
 {
+	static char usridbuf[L_cuserid];
 	struct passwd pw, *ppw;
 	long pwb[256];
 	if (getpwuid_r(geteuid(), &pw, (void *)pwb, sizeof pwb, &ppw))
 		return 0;
+	if (!buf) buf = usridbuf;
 	snprintf(buf, L_cuserid, "%s", pw.pw_name);
 	return buf;
 }

Re: [musl] [PATCH v2] cuserid: support invocation with a NULL pointer argument

[Sören Tempel]

Sorry, forgot to confirm my list subscription before sending the first
version of this patch. Just wanted to reply to a comment on the previous
version:

On Tue, 28 Jan 2020, Rich Felker wrote:
> I'm not sure whether to adopt this or not, but thanks for posting on
> the list for discussion. In any case it's something we should try to
> get fixed in apps that are using it, since this is no longer portable
> usage and is gratuitously thread-unsafe.

From my personal Alpine Linux packaging perspective: I would prefer
either removing the function entirely or supporting the invocation with
a NULL pointer argument. Currently, apps using this with a NULL pointer
(e.g. cdparanoia) will crash at runtime which makes it more difficult to
identify them. If the function would be removed entirely we would be
able to detect, during compilation, which apps use it and patch them
accordingly.

Cheers,
Sören

Re: [musl] [PATCH v2] cuserid: support invocation with a NULL pointer argument

[Sören Tempel]

Sören Tempel <soeren@soeren-tempel.net> wrote:
> From my personal Alpine Linux packaging perspective: I would prefer
> either removing the function entirely or supporting the invocation with
> a NULL pointer argument. Currently, apps using this with a NULL pointer
> (e.g. cdparanoia) will crash at runtime which makes it more difficult to
> identify them. If the function would be removed entirely we would be
> able to detect, during compilation, which apps use it and patch them
> accordingly.

Ping.

Greetings,
Sören

Re: [musl] [PATCH v2] cuserid: support invocation with a NULL pointer argument

[Rich Felker]

On Sun, Mar 22, 2020 at 10:51:56AM +0100, Sören Tempel wrote:
> Sören Tempel <soeren@soeren-tempel.net> wrote:
> > From my personal Alpine Linux packaging perspective: I would prefer
> > either removing the function entirely or supporting the invocation with
> > a NULL pointer argument. Currently, apps using this with a NULL pointer
> > (e.g. cdparanoia) will crash at runtime which makes it more difficult to
> > identify them. If the function would be removed entirely we would be
> > able to detect, during compilation, which apps use it and patch them
> > accordingly.
> 
> Ping.

Thanks. I'm trying to catch up on patches and I just confirmed your
research, so I think the patch is correct. I'll apply soon.

Rich

Re: [musl] [PATCH v2] cuserid: support invocation with a NULL pointer argument

[Rich Felker]

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

On Sun, Mar 22, 2020 at 11:05:57AM -0400, Rich Felker wrote:
> On Sun, Mar 22, 2020 at 10:51:56AM +0100, Sören Tempel wrote:
> > Sören Tempel <soeren@soeren-tempel.net> wrote:
> > > From my personal Alpine Linux packaging perspective: I would prefer
> > > either removing the function entirely or supporting the invocation with
> > > a NULL pointer argument. Currently, apps using this with a NULL pointer
> > > (e.g. cdparanoia) will crash at runtime which makes it more difficult to
> > > identify them. If the function would be removed entirely we would be
> > > able to detect, during compilation, which apps use it and patch them
> > > accordingly.
> > 
> > Ping.
> 
> Thanks. I'm trying to catch up on patches and I just confirmed your
> research, so I think the patch is correct. I'll apply soon.

Sorry for taking so long to get back to this. The function seems to
have been significantly broken on top of not supporting a null
argument. See attached three patches which I plan to push if there are
no stupid bugs in them.

Rich

[-- Attachment #2: 0001-cuserid-don-t-return-truncated-results.patch --]
[-- Type: text/plain, Size: 986 bytes --]

From a75283d777ed1827ed247dbb465818a0ce371c8f Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Sat, 13 Feb 2021 13:54:00 -0500
Subject: [PATCH 1/3] cuserid: don't return truncated results

checking the length also drops the need to pull in snprintf.
---
 src/legacy/cuserid.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/legacy/cuserid.c b/src/legacy/cuserid.c
index fd7832e4..3ff115a1 100644
--- a/src/legacy/cuserid.c
+++ b/src/legacy/cuserid.c
@@ -2,6 +2,7 @@
 #include <pwd.h>
 #include <stdio.h>
 #include <unistd.h>
+#include <string.h>
 
 char *cuserid(char *buf)
 {
@@ -10,7 +11,10 @@ char *cuserid(char *buf)
 	long pwb[256];
 	if (getpwuid_r(geteuid(), &pw, (void *)pwb, sizeof pwb, &ppw))
 		return 0;
+	size_t len = strnlen(pw.pw_name, L_cuserid);
+	if (len == L_cuserid)
+		return 0;
 	if (!buf) buf = usridbuf;
-	snprintf(buf, L_cuserid, "%s", pw.pw_name);
+	memcpy(buf, pw.pw_name, len+1);
 	return buf;
 }
-- 
2.21.0


[-- Attachment #3: 0002-fix-misuse-of-getpwuid_r-in-cuserid.patch --]
[-- Type: text/plain, Size: 993 bytes --]

From cc577d0e058b53df5c0fe2ac17890a41d77e94c5 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Sat, 13 Feb 2021 13:59:44 -0500
Subject: [PATCH 2/3] fix misuse of getpwuid_r in cuserid

getpwuid_r can return 0 but without a result in the case where there
was no error but no record exists. in that case cuserid was treating
it as success and copying junk out of pw.pw_name to the output buffer.
---
 src/legacy/cuserid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/legacy/cuserid.c b/src/legacy/cuserid.c
index 3ff115a1..07866acf 100644
--- a/src/legacy/cuserid.c
+++ b/src/legacy/cuserid.c
@@ -9,7 +9,8 @@ char *cuserid(char *buf)
 	static char usridbuf[L_cuserid];
 	struct passwd pw, *ppw;
 	long pwb[256];
-	if (getpwuid_r(geteuid(), &pw, (void *)pwb, sizeof pwb, &ppw))
+	getpwuid_r(geteuid(), &pw, (void *)pwb, sizeof pwb, &ppw);
+	if (!ppw)
 		return 0;
 	size_t len = strnlen(pw.pw_name, L_cuserid);
 	if (len == L_cuserid)
-- 
2.21.0


[-- Attachment #4: 0003-fix-error-return-value-for-cuserid.patch --]
[-- Type: text/plain, Size: 1100 bytes --]

From 49b6df3d9f3645de55607f1ac60095b22661b334 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Sat, 13 Feb 2021 14:03:23 -0500
Subject: [PATCH 3/3] fix error return value for cuserid

the historical function was specified to return an empty string in the
caller-provided buffer, not a null pointer, to indicate error when the
argument is non-null. only when the argument is null should it return
a null pointer on error.
---
 src/legacy/cuserid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/legacy/cuserid.c b/src/legacy/cuserid.c
index 07866acf..dcaf73d4 100644
--- a/src/legacy/cuserid.c
+++ b/src/legacy/cuserid.c
@@ -9,12 +9,13 @@ char *cuserid(char *buf)
 	static char usridbuf[L_cuserid];
 	struct passwd pw, *ppw;
 	long pwb[256];
+	if (buf) *buf = 0;
 	getpwuid_r(geteuid(), &pw, (void *)pwb, sizeof pwb, &ppw);
 	if (!ppw)
-		return 0;
+		return buf;
 	size_t len = strnlen(pw.pw_name, L_cuserid);
 	if (len == L_cuserid)
-		return 0;
+		return buf;
 	if (!buf) buf = usridbuf;
 	memcpy(buf, pw.pw_name, len+1);
 	return buf;
-- 
2.21.0