From 834b16684a7eee71a7a4dd0ef6e1eb25f3b034a2 Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Sun, 7 Dec 2025 00:13:22 +0100 Subject: [PATCH 1/5] lib/limits.c: Reduce scope of local arrays We only use them in the loop, and we write to them unconditionally in every iteration with sscanf(3), so we can reduce their lifetime, and skip the superfluous zeroing. Signed-off-by: Alejandro Colomar --- lib/limits.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/limits.c b/lib/limits.c index 4c7ac6ab1c..98d0fab23f 100644 --- a/lib/limits.c +++ b/lib/limits.c @@ -343,17 +343,13 @@ static int setup_user_limits (const char *uname) { FILE *fil; char buf[1024]; - char name[1024]; char limits[1024]; char deflimits[1024]; - char tempbuf[1024]; /* init things */ memzero_a(buf); - memzero_a(name); memzero_a(limits); memzero_a(deflimits); - memzero_a(tempbuf); /* start the checks */ fil = fopen (LIMITS_FILE, "r"); @@ -367,10 +363,12 @@ static int setup_user_limits (const char *uname) * FIXME: a better (smarter) checking should be done */ while (fgets (buf, 1024, fil) != NULL) { + char name[1024]; + char tempbuf[1024]; + if (strprefix(buf, "#") || strprefix(buf, "\n")) { continue; } - memzero_a(tempbuf); /* a valid line should have a username, then spaces, * then limits * we allow the format: From c7932f412f812ddd8dcd5a67a74172049c936809 Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Sun, 7 Dec 2025 00:17:41 +0100 Subject: [PATCH 2/5] lib/limits.c: Clear the newline And reject incomplete lines. Signed-off-by: Alejandro Colomar --- lib/limits.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/limits.c b/lib/limits.c index 98d0fab23f..73e253c501 100644 --- a/lib/limits.c +++ b/lib/limits.c @@ -35,6 +35,7 @@ #include "string/strcmp/streq.h" #include "string/strcmp/strprefix.h" #include "string/strspn/stpspn.h" +#include "string/strtok/stpsep.h" #include "typetraits.h" @@ -366,9 +367,10 @@ static int setup_user_limits (const char *uname) char name[1024]; char tempbuf[1024]; - if (strprefix(buf, "#") || strprefix(buf, "\n")) { + if (stpsep(buf, "\n") == NULL) + continue; + if (strprefix(buf, "#") || streq(buf, "")) continue; - } /* a valid line should have a username, then spaces, * then limits * we allow the format: From 0abbd660c33d0ce21ad5f2c23ca91a2961efe643 Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Sun, 7 Dec 2025 00:26:14 +0100 Subject: [PATCH 3/5] lib/limits.c: Replace sscanf(3) with conventional string parsing sscanf(3) is quite brittle. Let's parse with regular string functions, which are much more robust. This should silence a CodeQL false positive, BTW. This new parsing has a slight difference with the sscanf(3) call: the first whitespace after the name is turned into a null delimiter byte, while sscanf(3) was keeping it as part of the second string. However, since we later skip leading white space --within do_user_limits()--, this is irrelevant. I've tested this patch with the following program: $ cat sscanf.c #include #include #include extern inline char * stpsep(char *s, const char *delim) { strsep(&s, delim); return s; } #define stpspn(s, accept) \ ({ \ auto s_ = s; \ \ s_ + strspn(s_, accept); \ }) int main(void) { char buf[1000]; char s1[1000] = ""; char s2[1000] = ""; char *p1, *p2; int n; if (fgets(buf, 1000, stdin) == NULL) err(1, "fgets"); n = sscanf(buf, "%s%[ACDFIKLMNOPRSTUacdfiklmnoprstu0-9 \t-]", s1, s2); if (n != 2) warn("sscanf: %d", n); printf("--- s1: |%s|\n", s1); printf("--- s2: |%s|\n", s2); if (stpsep(buf, "\n") == NULL) warn("stpsep(\"\n\")"); p1 = buf; p2 = stpsep(buf, " \t"); if (p2 == NULL) warn("stpsep"); else stpcpy(stpspn(p2, "ACDFIKLMNOPRSTUacdfiklmnoprstu0123456789 \t-"), ""); printf("+++ s1: |%s|\n", p1); printf("+++ s2: |%s|\n", p2); } alx@devuan:~/tmp$ gcc -Wall -Wextra sscanf.c alx@devuan:~/tmp$ ./a.out qwe a.out: sscanf: 1: Success --- s1: |qwe| --- s2: || a.out: stpsep: Success +++ s1: |qwe| +++ s2: |(null)| alx@devuan:~/tmp$ ./a.out qwe qwe --- s1: |qwe| --- s2: | | +++ s1: |qwe| +++ s2: || alx@devuan:~/tmp$ ./a.out qwe acd acd 123456 --- s1: |qwe| --- s2: | acd acd 123456| +++ s1: |qwe| +++ s2: |acd acd 123456| alx@devuan:~/tmp$ ./a.out qwe acd123 qwe --- s1: |qwe| --- s2: | acd123 | +++ s1: |qwe| +++ s2: |acd123 | Interestingly, this patch makes it more obvious that we're silently ignoring garbage after the limit. Should we be more strict in what we parse? That'll be for another day. BTW, we've removed a conditional, but keep the code indented. That will keep this patch small. The next commit will fix that indentation. Signed-off-by: Alejandro Colomar --- lib/limits.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/limits.c b/lib/limits.c index 73e253c501..9b788e14c7 100644 --- a/lib/limits.c +++ b/lib/limits.c @@ -364,8 +364,7 @@ static int setup_user_limits (const char *uname) * FIXME: a better (smarter) checking should be done */ while (fgets (buf, 1024, fil) != NULL) { - char name[1024]; - char tempbuf[1024]; + char *name, *lim, *p; if (stpsep(buf, "\n") == NULL) continue; @@ -391,23 +390,27 @@ static int setup_user_limits (const char *uname) * the last encountered entry for a matching group rules. * If there is no matching group entry, the default limits rule. */ - if (sscanf (buf, "%s%[ACDFIKLMNOPRSTUacdfiklmnoprstu0-9 \t-]", - name, tempbuf) == 2) { + name = buf; + lim = stpsep(buf, " \t"); + if (lim == NULL) + continue; + p = stpspn(lim, "ACDFIKLMNOPRSTUacdfiklmnoprstu0123456789 \t-"); + stpcpy(p, ""); + if (streq(name, uname)) { - strcpy (limits, tempbuf); + strcpy (limits, lim); break; } else if (streq(name, "*")) { - strcpy (deflimits, tempbuf); + strcpy (deflimits, lim); } else if (strprefix(name, "@")) { /* If the user is in the group, the group * limits apply unless later a line for * the specific user is found. */ if (user_in_group (uname, name+1)) { - strcpy (limits, tempbuf); + strcpy (limits, lim); } } - } } (void) fclose (fil); if (streq(limits, "")) { From a57f58cd0eb41d6e56a198bcf58920eab474aae0 Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Sun, 7 Dec 2025 00:27:37 +0100 Subject: [PATCH 4/5] lib/limits.c: Unindent Signed-off-by: Alejandro Colomar --- lib/limits.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/limits.c b/lib/limits.c index 9b788e14c7..8ec50d8d85 100644 --- a/lib/limits.c +++ b/lib/limits.c @@ -397,20 +397,20 @@ static int setup_user_limits (const char *uname) p = stpspn(lim, "ACDFIKLMNOPRSTUacdfiklmnoprstu0123456789 \t-"); stpcpy(p, ""); - if (streq(name, uname)) { - strcpy (limits, lim); - break; - } else if (streq(name, "*")) { - strcpy (deflimits, lim); - } else if (strprefix(name, "@")) { - /* If the user is in the group, the group - * limits apply unless later a line for - * the specific user is found. - */ - if (user_in_group (uname, name+1)) { - strcpy (limits, lim); - } + if (streq(name, uname)) { + strcpy(limits, lim); + break; + } else if (streq(name, "*")) { + strcpy(deflimits, lim); + } else if (strprefix(name, "@")) { + /* If the user is in the group, the group + * limits apply unless later a line for + * the specific user is found. + */ + if (user_in_group(uname, name+1)) { + strcpy(limits, lim); } + } } (void) fclose (fil); if (streq(limits, "")) { From dfba6c46e68c64105bba6b6b1dd3c68af58fe42e Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Sun, 7 Dec 2025 01:23:54 +0100 Subject: [PATCH 5/5] lib/limits.c: Remove dead code We overwrite this in fgets(3), and even if it were to fail in the first call, we don't use the buffer after that, so the zeroing is superfluous. Signed-off-by: Alejandro Colomar --- lib/limits.c | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/limits.c b/lib/limits.c index 8ec50d8d85..5007f869d5 100644 --- a/lib/limits.c +++ b/lib/limits.c @@ -348,7 +348,6 @@ static int setup_user_limits (const char *uname) char deflimits[1024]; /* init things */ - memzero_a(buf); memzero_a(limits); memzero_a(deflimits);