[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-12 08:28, Alejandro Colomar wrote: I've pushed a signed tag paul1, so you can safely check that the repo is mine (since I don't have HTTPS). Thanks, I'm not sure what exactly this means as I don't contribute to shadow-devel. As far as the remaining patches go, please use your best judgment as I'm running low on time to worry about this. ___ Pkg-shadow-devel mailing list Pkg-shadow-devel@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-11 14:02, Alejandro Colomar wrote:
we should use "%s" (if we go the way of snprintf(3)).
Yes, thanks for catching that. However, I came up with a better way that
avoids snprintf (and strlcpy) entirely both here and the other place I
used snprintf.
Attached is a revised set of patches that addresses the comments you
made and embodies my followups.From 324bb0e914b5470650df02bd7b64e963665d44c1 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 00:01:02 -0800
Subject: [PATCH 1/8] Simplify change_field by using strcpy
* lib/fields.c (change_field): Since we know the string fits,
use strcpy rather than strlcpy.
Signed-off-by: Paul Eggert
---
lib/fields.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/fields.c b/lib/fields.c
index 0b5f91b2..8801bce6 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -100,7 +100,6 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
cp++;
}
- strlcpy (buf, cp, maxsize);
+ strcpy (buf, cp);
}
}
-
--
2.37.2
From b13ffb86dcd10e8160eee10bd286fc73937c3e8b Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 13:41:54 -0800
Subject: [PATCH 2/8] Omit unneeded change_field test
* fields.c (change_field): Omit unnecessary test.
Signed-off-by: Paul Eggert
---
lib/fields.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/fields.c b/lib/fields.c
index 8801bce6..fa5fd156 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -96,7 +96,7 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
*cp = '\0';
cp = newf;
- while (('\0' != *cp) && isspace (*cp)) {
+ while (isspace (*cp)) {
cp++;
}
--
2.37.2
From 090722a20765cf9a248050524143fce5b68cfe8c Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 13:43:36 -0800
Subject: [PATCH 3/8] Fix change_field buffer underrun
* lib/fields.c (change_field): Don't point
before array start; that has undefined behavior.
Signed-off-by: Paul Eggert
---
lib/fields.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/fields.c b/lib/fields.c
index fa5fd156..640be931 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -91,8 +91,9 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
* entering a space. --marekm
*/
- while (--cp >= newf && isspace (*cp));
- cp++;
+ while (newf < cp && isspace (cp[-1])) {
+ cp--;
+ }
*cp = '\0';
cp = newf;
--
2.37.2
From 4982d5f2fe2f2c339568996ebb17a99200d2f106 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 00:02:45 -0800
Subject: [PATCH 4/8] Prefer strcpy to strlcpy when either works
* lib/gshadow.c (sgetsgent): Use strcpy not strlcpy,
since the string is known to fit.
Signed-off-by: Paul Eggert
---
lib/gshadow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/gshadow.c b/lib/gshadow.c
index c17af67f..ca14449a 100644
--- a/lib/gshadow.c
+++ b/lib/gshadow.c
@@ -128,7 +128,7 @@ void endsgent (void)
sgrbuflen = len;
}
- strlcpy (sgrbuf, string, len);
+ strcpy (sgrbuf, string);
cp = strrchr (sgrbuf, '\n');
if (NULL != cp) {
--
2.37.2
From 54fac7560f87a134c4d3045ce7048f4819c4e492 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 00:38:24 -0800
Subject: [PATCH 5/8] Avoid silent truncation of console file data
* libmisc/console.c (is_listed): Rework so that there is no
fixed-size buffer, and no need to use fgets or strlcpy or strtok.
Instead, the code works with arbitrary-sized input,
without silently truncating data or mishandling NUL
bytes in the console file.
Signed-off-by: Paul Eggert
---
libmisc/console.c | 41 -
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/libmisc/console.c b/libmisc/console.c
index 7e2132dd..8264e1a3 100644
--- a/libmisc/console.c
+++ b/libmisc/console.c
@@ -24,7 +24,6 @@
static bool is_listed (const char *cfgin, const char *tty, bool def)
{
FILE *fp;
- char buf[1024], *s;
const char *cons;
/*
@@ -43,17 +42,17 @@ static bool is_listed (const char *cfgin, const char *tty, bool def)
*/
if (*cons != '/') {
- char *pbuf;
- strlcpy (buf, cons, sizeof (buf));
- pbuf = &buf[0];
- while ((s = strtok (pbuf, ":")) != NULL) {
- if (strcmp (s, tty) == 0) {
+ size_t ttylen = strlen (tty);
+ for (;;) {
+ if (strncmp (cons, tty, ttylen) == 0
+ && (cons[ttylen] == ':' || !cons[ttylen])) {
return true;
}
-
- pbuf = NULL;
+ cons = strchr (cons, ':');
+ if (!cons)
+return false;
+ cons++;
}
- return false;
}
/*
@@ -70,21 +69,22 @@ static bool is_listed (const char *cfgin, const char *tty, bool def)
* See if this tty is listed in the console file.
*/
- while (fgets (buf, sizeof (buf), fp) != NULL) {
- /* Remove optional trailing '\n'. */
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-11 14:39, Alejandro Colomar wrote: I wonder if the patch is really "simplifying". It depends on how one measures simplicity. The reader will need to know strftime's API regardless; requiring the reader to also know strlcpy's API makes the reader's job harder. Also, it's less machine code to call just the one function (if one cares about simplicity of debugging :-). If you still prefer calling two different functions instead of just one, feel free to modify it to use plain strcpy. strlcpy isn't needed here as the destination buffers are all big enough. To be honest though I like it the way it is (though it could use a comment; I'll add that). ___ Pkg-shadow-devel mailing list Pkg-shadow-devel@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-11 13:49, Alejandro Colomar wrote: +: mempcpy (full_tty, "/dev/", sizeof"/dev/" - 1)), This is a great use case for stpcpy(3). I came up with a slightly better approach, that needs neither mempcpy nor stpcpy. I plan to send it along soon. ___ Pkg-shadow-devel mailing list Pkg-shadow-devel@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-11 13:59, Alejandro Colomar wrote: If the function is allowed to dereference, then NULL is not allowed, but if the values are uninitialized, then reading any of them should also trigger UB, no? Sure, but the standard says that strftime reads only the struct tm members needed to interpret the format. If the format contains no conversion specs, strftime reads no struct tm members and thus there is no UB even if the struct tm is entirely uninitialized. ___ Pkg-shadow-devel mailing list Pkg-shadow-devel@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-11 14:18, Alejandro Colomar wrote: What I'm not sure is that strftime(3) requires nonnull. glibc's strftime implementation segfaults if you pass a null pointer, so we can't pass NULL regardless of whether the strftime API in time.h uses __attribute__ ((nonnull))'. ___ Pkg-shadow-devel mailing list Pkg-shadow-devel@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-11 13:59, Alejandro Colomar wrote: Unless the standard specifically allows us to do so, but I can't find anything clear. It's pretty clear if you're a time nerd like me. :-) The standard for strftime says "The appropriate characters are determined using the LC_TIME category of the current locale and by the values of zero or more members of the broken-down time structure pointed to by timeptr, as specified in brackets in the description. If any of the specified values are outside the normal range, the characters stored are unspecified." The "zero" means that if no conversion specs are present in the format string, then no struct tm members are examined, and it's therefore OK for all members to be uninitialized if no conversion specs are present. ___ Pkg-shadow-devel mailing list Pkg-shadow-devel@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
On 2023-03-11 13:31, Alejandro Colomar wrote: What's this &dummy exactly for? It avoids undefined behavior. A call like strftime (buf, sizeof buf, "XXX", NULL) has undefined behavior, as near as I can make out. It's OK that the dummy is uninitialized. ___ Pkg-shadow-devel mailing list Pkg-shadow-devel@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel
[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
I looked into this, and five of the shadow package's six uses of strlcpy
are wrong, i.e., they are associated with silent truncation or buffer
overrun/underrun or dereferencing NULL in nearby code. This isn't
surprising, as strlcpy is commonly used in code that has been
slapdashedly hacked to try to make it safer, and in my experience code
that that has been modified in that way is usually wrong.
Proposed patches attached.
Although there is one correct use of strlcpy, the correct use (in
sgetsgent) is equivalent to memcpy so there is no need for strlcpy there
(see patch 0002).
From d40e2f92f3e50d13d87393bd30b2b4b20b89a2d6 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 00:01:02 -0800
Subject: [PATCH 1/6] Fix undefined behavior in change_field
* lib/fields.c (change_field): Do not ever compute &newf[-1],
as behavior is undefined. Since we know that the string fits,
use memcpy rather than strlcpy.
Signed-off-by: Paul Eggert
---
lib/fields.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/lib/fields.c b/lib/fields.c
index 0b5f91b2..3b119502 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -90,17 +90,17 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
* makes it possible to change the field to empty, by
* entering a space. --marekm
*/
+ char *bp = newf;
- while (--cp >= newf && isspace (*cp));
- cp++;
+ while (newf < cp && isspace (cp[-1])) {
+ cp--;
+ }
*cp = '\0';
- cp = newf;
- while (('\0' != *cp) && isspace (*cp)) {
- cp++;
+ while (isspace (*bp)) {
+ bp++;
}
- strlcpy (buf, cp, maxsize);
+ memcpy (buf, bp, cp + 1 - bp);
}
}
-
--
2.37.2
From 7e88c5914c1fab6c4d88e1ca39d6b6319e7ee768 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 00:02:45 -0800
Subject: [PATCH 2/6] Prefer memcpy to strlcpy when either works
memcpy is standardized and should be faster here.
* lib/gshadow.c (sgetsgent): Use memcpy not strlcpy,
since the string is known to fit.
Signed-off-by: Paul Eggert
---
lib/gshadow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/gshadow.c b/lib/gshadow.c
index c17af67f..1976c9a9 100644
--- a/lib/gshadow.c
+++ b/lib/gshadow.c
@@ -128,7 +128,7 @@ void endsgent (void)
sgrbuflen = len;
}
- strlcpy (sgrbuf, string, len);
+ memcpy (sgrbuf, string, len);
cp = strrchr (sgrbuf, '\n');
if (NULL != cp) {
--
2.37.2
From a1c2e046d52042cf60ff7196a9d9a972573290bd Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 11 Mar 2023 00:38:24 -0800
Subject: [PATCH 3/6] Avoid silent truncation of console file data
* libmisc/console.c (is_listed): Rework so that there is no
fixed-size buffer, and no need to use fgets or strlcpy or strtok.
Instead, the code works with arbitrary-sized input,
without silently truncating data or mishandling NUL
bytes in the console file.
Signed-off-by: Paul Eggert
---
libmisc/console.c | 41 -
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/libmisc/console.c b/libmisc/console.c
index 7e2132dd..8264e1a3 100644
--- a/libmisc/console.c
+++ b/libmisc/console.c
@@ -24,7 +24,6 @@
static bool is_listed (const char *cfgin, const char *tty, bool def)
{
FILE *fp;
- char buf[1024], *s;
const char *cons;
/*
@@ -43,17 +42,17 @@ static bool is_listed (const char *cfgin, const char *tty, bool def)
*/
if (*cons != '/') {
- char *pbuf;
- strlcpy (buf, cons, sizeof (buf));
- pbuf = &buf[0];
- while ((s = strtok (pbuf, ":")) != NULL) {
- if (strcmp (s, tty) == 0) {
+ size_t ttylen = strlen (tty);
+ for (;;) {
+ if (strncmp (cons, tty, ttylen) == 0
+ && (cons[ttylen] == ':' || !cons[ttylen])) {
return true;
}
-
- pbuf = NULL;
+ cons = strchr (cons, ':');
+ if (!cons)
+return false;
+ cons++;
}
- return false;
}
/*
@@ -70,21 +69,22 @@ static bool is_listed (const char *cfgin, const char *tty, bool def)
* See if this tty is listed in the console file.
*/
- while (fgets (buf, sizeof (buf), fp) != NULL) {
- /* Remove optional trailing '\n'. */
- buf[strcspn (buf, "\n")] = '\0';
- if (strcmp (buf, tty) == 0) {
- (void) fclose (fp);
- return true;
+ const char *tp = tty;
+ bool listed = false;
+ for (int c; 0 <= (c = getc (fp)); ) {
+ if (c == '\n') {
+ if (tp && !*tp) {
+listed = true;
+break;
+ }
+ tp = tty;
+ } else if (tp) {
+ tp = *tp == c && c ? tp + 1 : NULL;
}
}
- /*
- * This tty isn't a console.
- */
-
(void) fclose (fp);
- return false;
+ return listed;
}
/*
@@ -105,4 +105,3 @@ bool console (const char *tty)
return is_listed ("CONSOLE", tty, true);
}
-
--
2.37.2
From 1c8388d1d1831e976cdaa6e6f27bb08bf31aedc5 Mon Sep 17 00:00:00 2001
From: Paul
