Replace one sscanf(3) call by conventional string parsing by alejandro-colomar · Pull Request #1408 · shadow-maint/shadow

alejandro-colomar · 2025-12-07T00:10:22Z

Revisions:

v1b

Rebase
Reword commit message.

$ git rd 
1:  1df03082f ! 1:  d8c60e2d1 lib/limits.c: Reduce scope of local arrays
    @@ lib/limits.c: static int setup_user_limits (const char *uname)
        /* start the checks */
        fil = fopen (LIMITS_FILE, "r");
     @@ lib/limits.c: static int setup_user_limits (const char *uname)
    -    * FIXME: A better (smarter) checking should be done
    +    * FIXME: a better (smarter) checking should be done
         */
        while (fgets (buf, 1024, fil) != NULL) {
     +          char  name[1024];
2:  a1ad7bae3 = 2:  ddc2186c7 lib/limits.c: Clear the newline
3:  8cb6e30fb ! 3:  e4264669e lib/limits.c: Replace sscanf(3) with conventional string parsing
    @@ Commit message
     
      ## lib/limits.c ##
     @@ lib/limits.c: static int setup_user_limits (const char *uname)
    -    * FIXME: A better (smarter) checking should be done
    +    * FIXME: a better (smarter) checking should be done
         */
        while (fgets (buf, 1024, fil) != NULL) {
     -          char  name[1024];
4:  67f545f11 = 4:  231bf262c lib/limits.c: Unindent
5:  99792bc90 ! 5:  fb7418036 lib/limits.c: Remove dead code
    @@ Metadata
      ## Commit message ##
         lib/limits.c: Remove dead code
     
    -    We're going to 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.
    +    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 <alx@kernel.org>

v1c

Rebase

$ git rd 
1:  d8c60e2d1 = 1:  834b16684 lib/limits.c: Reduce scope of local arrays
2:  ddc2186c7 = 2:  c7932f412 lib/limits.c: Clear the newline
3:  e4264669e = 3:  0abbd660c lib/limits.c: Replace sscanf(3) with conventional string parsing
4:  231bf262c = 4:  a57f58cd0 lib/limits.c: Unindent
5:  fb7418036 = 5:  dfba6c46e lib/limits.c: Remove dead code

v1d

Rebase

$ git rd 
1:  834b16684a7e = 1:  e7dcccee8462 lib/limits.c: Reduce scope of local arrays
2:  c7932f412f81 = 2:  d0f5300ba018 lib/limits.c: Clear the newline
3:  0abbd660c33d = 3:  ef28002316db lib/limits.c: Replace sscanf(3) with conventional string parsing
4:  a57f58cd0eb4 = 4:  a8893ea2572e lib/limits.c: Unindent
5:  dfba6c46e68c = 5:  f5d0a8954859 lib/limits.c: Remove dead code

v2

Remove unnecessary local variable.

$ git rd 
1:  e7dcccee8462 = 1:  e7dcccee8462 lib/limits.c: Reduce scope of local arrays
2:  d0f5300ba018 = 2:  d0f5300ba018 lib/limits.c: Clear the newline
3:  ef28002316db ! 3:  80bbc3dcf6bf lib/limits.c: Replace sscanf(3) with conventional string parsing
    @@ lib/limits.c: static int setup_user_limits (const char *uname)
        while (fgets (buf, 1024, fil) != NULL) {
     -          char  name[1024];
     -          char  tempbuf[1024];
    -+          char  *name, *lim, *p;
    ++          char  *name, *lim;
      
                if (stpsep(buf, "\n") == NULL)
                        continue;
    @@ lib/limits.c: static int setup_user_limits (const char *uname)
     +          lim = stpsep(buf, " \t");
     +          if (lim == NULL)
     +                  continue;
    -+          p = stpspn(lim, "ACDFIKLMNOPRSTUacdfiklmnoprstu0123456789 \t-");
    -+          stpcpy(p, "");
    ++          stpcpy(stpspn(lim, "ACDFIKLMNOPRSTUacdfiklmnoprstu0123456789 \t-"), "");
     +
                        if (streq(name, uname)) {
     -                          strcpy (limits, tempbuf);
4:  a8893ea2572e ! 4:  5a159f02ebad lib/limits.c: Unindent
    @@ Commit message
     
      ## lib/limits.c ##
     @@ lib/limits.c: static int setup_user_limits (const char *uname)
    -           p = stpspn(lim, "ACDFIKLMNOPRSTUacdfiklmnoprstu0123456789 \t-");
    -           stpcpy(p, "");
    +                   continue;
    +           stpcpy(stpspn(lim, "ACDFIKLMNOPRSTUacdfiklmnoprstu0123456789 \t-"), "");
      
     -                  if (streq(name, uname)) {
     -                          strcpy (limits, lim);
5:  f5d0a8954859 = 5:  a9e0003b8bfe lib/limits.c: Remove dead code

v2b

Rebase

$ git rd 
1:  e7dcccee = 1:  248581fa lib/limits.c: Reduce scope of local arrays
2:  d0f5300b = 2:  8c3e5818 lib/limits.c: Clear the newline
3:  80bbc3dc = 3:  e0afb375 lib/limits.c: Replace sscanf(3) with conventional string parsing
4:  5a159f02 = 4:  e8f7da53 lib/limits.c: Unindent
5:  a9e0003b = 5:  9f7660e7 lib/limits.c: Remove dead code

v2c

Rebase

$ git rd -U0
1:  248581fa ! 1:  16eb4e49 lib/limits.c: Reduce scope of local arrays
    @@ lib/limits.c: static int setup_user_limits (const char *uname)
    -   while (fgets (buf, 1024, fil) != NULL) {
    +   while (fgets_a(buf, fil) != NULL) {
2:  8c3e5818 = 2:  2f82627b lib/limits.c: Clear the newline
3:  e0afb375 ! 3:  5d201ba7 lib/limits.c: Replace sscanf(3) with conventional string parsing
    @@ lib/limits.c: static int setup_user_limits (const char *uname)
    -   while (fgets (buf, 1024, fil) != NULL) {
    +   while (fgets_a(buf, fil) != NULL) {
4:  e8f7da53 = 4:  930a93a1 lib/limits.c: Unindent
5:  9f7660e7 = 5:  12001dc5 lib/limits.c: Remove dead code

alejandro-colomar · 2026-01-08T02:34:36Z

Cc: @kees

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 <alx@kernel.org>

And reject incomplete lines. Signed-off-by: Alejandro Colomar <alx@kernel.org>

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 <err.h> #include <stdio.h> #include <string.h> 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 <alx@kernel.org>

Signed-off-by: Alejandro Colomar <alx@kernel.org>

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 <alx@kernel.org>

alejandro-colomar force-pushed the sscanf branch 4 times, most recently from a44624a to 67f545f Compare December 7, 2025 00:18

github-advanced-security AI found potential problems Dec 7, 2025

View reviewed changes

Comment thread lib/limits.c Dismissed

Comment thread lib/limits.c Dismissed

Comment thread lib/limits.c Dismissed

alejandro-colomar requested review from hallyn and ikerexxe December 7, 2025 00:41

alejandro-colomar self-assigned this Dec 11, 2025

alejandro-colomar force-pushed the sscanf branch from 99792bc to fb74180 Compare December 23, 2025 23:59

alejandro-colomar force-pushed the sscanf branch from fb74180 to dfba6c4 Compare December 31, 2025 02:45

alejandro-colomar force-pushed the sscanf branch 2 times, most recently from f5d0a89 to a9e0003 Compare February 23, 2026 13:54

alejandro-colomar marked this pull request as draft February 23, 2026 13:55

alejandro-colomar marked this pull request as ready for review February 23, 2026 13:57

alejandro-colomar force-pushed the sscanf branch from a9e0003 to 9f7660e Compare March 4, 2026 16:30

alejandro-colomar added 5 commits April 12, 2026 09:29

lib/limits.c: Reduce scope of local arrays

16eb4e4

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 <alx@kernel.org>

lib/limits.c: Clear the newline

2f82627

And reject incomplete lines. Signed-off-by: Alejandro Colomar <alx@kernel.org>

lib/limits.c: Unindent

930a93a

Signed-off-by: Alejandro Colomar <alx@kernel.org>

lib/limits.c: Remove dead code

12001dc

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 <alx@kernel.org>

alejandro-colomar force-pushed the sscanf branch from 9f7660e to 12001dc Compare April 12, 2026 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace one sscanf(3) call by conventional string parsing#1408

Replace one sscanf(3) call by conventional string parsing#1408
alejandro-colomar wants to merge 5 commits intoshadow-maint:masterfrom
alejandro-colomar:sscanf

alejandro-colomar commented Dec 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alejandro-colomar commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

alejandro-colomar commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alejandro-colomar commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alejandro-colomar commented Dec 7, 2025 •

edited

Loading