From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 26463 invoked from network); 13 Feb 2021 19:06:52 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 13 Feb 2021 19:06:52 -0000 Received: (qmail 17703 invoked by uid 550); 13 Feb 2021 19:06:49 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 17684 invoked from network); 13 Feb 2021 19:06:48 -0000 Date: Sat, 13 Feb 2021 14:06:36 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20210213190634.GC11590@brightrain.aerifal.cx> References: <20200129112007.17575-1-soeren+git@soeren-tempel.net> <2NSST1D2CU3ZF.2X6OJSUS9Q20S@8pit.net> <36221X68MA893.2BYWBI9CL1WMJ@8pit.net> <20200322150557.GN11469@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="T4sUOijqQbZv57TR" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200322150557.GN11469@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH v2] cuserid: support invocation with a NULL pointer argument --T4sUOijqQbZv57TR Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 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 --T4sUOijqQbZv57TR Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-cuserid-don-t-return-truncated-results.patch" >From a75283d777ed1827ed247dbb465818a0ce371c8f Mon Sep 17 00:00:00 2001 From: Rich Felker 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 #include #include +#include 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 --T4sUOijqQbZv57TR Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0002-fix-misuse-of-getpwuid_r-in-cuserid.patch" >From cc577d0e058b53df5c0fe2ac17890a41d77e94c5 Mon Sep 17 00:00:00 2001 From: Rich Felker 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 --T4sUOijqQbZv57TR Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0003-fix-error-return-value-for-cuserid.patch" >From 49b6df3d9f3645de55607f1ac60095b22661b334 Mon Sep 17 00:00:00 2001 From: Rich Felker 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 --T4sUOijqQbZv57TR--