Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Moar string stuff: snprintf(3) #796

Closed
wants to merge 10 commits into from
Closed

Moar string stuff: snprintf(3) #796

wants to merge 10 commits into from

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Sep 1, 2023 •
edited
Loading

Merge after #793 (done)

@alejandro-colomar alejandro-colomar changed the title Moar string stuff Moar string stuff: snprintf(3) Sep 1, 2023
@alejandro-colomar alejandro-colomar force-pushed the snprintf branch 4 times, most recently from d516d42 to 14c2c46 Compare September 1, 2023 15:13
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 1, 2023
View reviewed changes
lib/run_part.c Fixed Show fixed Hide fixed
lib/pwauth.c Fixed Show resolved Hide resolved
lib/commonio.c Fixed Show fixed Hide fixed
lib/get_pid.c Fixed Show fixed Hide fixed
src/useradd.c Fixed Show fixed Hide fixed
@alejandro-colomar alejandro-colomar force-pushed the snprintf branch 5 times, most recently from afbbae7 to cdba035 Compare September 6, 2023 09:23
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 6, 2023
View reviewed changes
lib/pwauth.c Dismissed Show resolved Hide resolved
@alejandro-colomar alejandro-colomar force-pushed the snprintf branch 2 times, most recently from fbfe7a7 to 71192a6 Compare October 4, 2023 17:00
@alejandro-colomar alejandro-colomar force-pushed the snprintf branch 4 times, most recently from 0e2232e to 54a076a Compare October 20, 2023 19:09
@alejandro-colomar
Copy link
Collaborator Author

v2 changes:

  • Rebase to master
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  9af7b110 =  1:  8c6b3936 lib/, src/: snprintf(3) already terminates strings with NUL
 2:  4e4d4910 =  2:  6efd06ec lib/sprintf.h: Add SNPRINTF() macro
 3:  3bad3d93 =  3:  b25d2101 src/login.c: Group preprocessor conditionals
 4:  7a1f27bc =  4:  8e18811e lib/, src/: Align variable definitions
 5:  4cc56dd5 !  5:  d325eb67 lib/, src/: Use SNPRINTF() instead of its pattern
    @@ src/chage.c
      #include "shadowio.h"
      #include "shadowlog.h"
     +#include "sprintf.h"
    - #include "strlcpy.h"
    + #include "strtcpy.h"
      #ifdef WITH_TCB
      #include "tcbfuncs.h"
     @@
    @@ src/chfn.c
      #include "exitcodes.h"
      #include "shadowlog.h"
     +#include "sprintf.h"
    - #include "strlcpy.h"
    + #include "strtcpy.h"
      
     +
      /*
    @@ src/gpasswd.c
      #include "exitcodes.h"
      #include "shadowlog.h"
     +#include "sprintf.h"
    - #include "strlcpy.h"
    + #include "strtcpy.h"
      
     +
      /*
    @@ src/login.c
      #include "exitcodes.h"
      #include "shadowlog.h"
     +#include "sprintf.h"
    - #include "strlcpy.h"
    + #include "strtcpy.h"
      
      #ifdef USE_PAM
     @@ src/login.c: static /*@observer@*/const char *get_failent_user (/*@returned@*/const char *use
    @@ src/login.c: int main (int argc, char **argv)
     -                            _("%s login: "), hostn);
     +                  SNPRINTF(loginprompt, _("%s login: "), hostn);
                } else {
    -                   STRLCPY(loginprompt, _("login: "));
    +                   STRTCPY(loginprompt, _("login: "));
                }
     
      ## src/newgrp.c ##
    @@ src/su.c
      #include "exitcodes.h"
      #include "shadowlog.h"
     +#include "sprintf.h"
    - #include "strlcpy.h"
    + #include "strtcpy.h"
      
     +
      /*
 6:  316984d3 =  6:  21230bec lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  d9a9ba04 =  7:  21b51714 lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  86608fe8 =  8:  ed7a6de1 lib/sprintf.[ch]: Add [v]snprintf_()
 9:  4c53738a =  9:  687285ab lib/: Use snprintf_() where appropriate

@alejandro-colomar alejandro-colomar marked this pull request as ready for review November 22, 2023 12:57
@alejandro-colomar
Copy link
Collaborator Author

v3:

  • Add tests for SNPRINTF().
$ git log --oneline shadow/master..snprintf 
dd9a7d93 (HEAD -> snprintf, gh/snprintf) tests/unit/test_snprintf.c: Test SNPRINTF()
687285ab lib/: Use snprintf_() where appropriate
ed7a6de1 lib/sprintf.[ch]: Add [v]snprintf_()
21b51714 lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
21230bec lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
d325eb67 lib/, src/: Use SNPRINTF() instead of its pattern
8e18811e lib/, src/: Align variable definitions
b25d2101 src/login.c: Group preprocessor conditionals
6efd06ec lib/sprintf.h: Add SNPRINTF() macro
8c6b3936 lib/, src/: snprintf(3) already terminates strings with NUL

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Nov 22, 2023 •
edited
Loading

v3 changes:

  • Use NITEMS() instead of SIZEOF_ARRAY(). It's more appropriate (think of swprintf(3)).
  • Update references to STRLCPY() with STRTCPY().
$ git range-diff shadow/master gh/snprintf snprintf 
 1:  8c6b3936 =  1:  8c6b3936 lib/, src/: snprintf(3) already terminates strings with NUL
 2:  6efd06ec !  2:  61fb6205 lib/sprintf.h: Add SNPRINTF() macro
    @@ Commit message
     
         BTW, this macro doesn't have any issues of double evaluation, because
         sizeof() doesn't evaluate its argument (unless it's a VLA, but then the
    -    static_assert(3) within SIZEOF_ARRAY() makes sure VLAs are not allowed).
    +    static_assert(3) within NITEMS() makes sure VLAs are not allowed).
     
    -    This macro is very similar to STRLCPY(), defined in lib/strlcpy.h.
    +    This macro is very similar to STRTCPY(), defined in lib/strtcpy.h.
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    @@ lib/sprintf.h
     +({                                                                            \
     +  size_t  sz_, len_;                                                    \
     +                                                                              \
    -+  sz_ = SIZEOF_ARRAY(s);                                                \
    ++  sz_ = NITEMS(s);                                                      \
     +  len_ = snprintf(s, sz_, fmt __VA_OPT__(,) __VA_ARGS__);               \
     +                                                                              \
     +  (len_ >= sz_) ? -1 : len_;                                            \
 3:  b25d2101 =  3:  178a764e src/login.c: Group preprocessor conditionals
 4:  8e18811e =  4:  da76b88f lib/, src/: Align variable definitions
 5:  d325eb67 =  5:  55587bcc lib/, src/: Use SNPRINTF() instead of its pattern
 6:  21230bec =  6:  9bece4b9 lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  21b51714 =  7:  06134c94 lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  ed7a6de1 !  8:  71725088 lib/sprintf.[ch]: Add [v]snprintf_()
    @@ lib/sprintf.h
     -({                                                                            \
     -  size_t  sz_, len_;                                                    \
     -                                                                              \
    --  sz_ = SIZEOF_ARRAY(s);                                                \
    +-  sz_ = NITEMS(s);                                                      \
     -  len_ = snprintf(s, sz_, fmt __VA_OPT__(,) __VA_ARGS__);               \
     -                                                                              \
     -  (len_ >= sz_) ? -1 : len_;                                            \
     -})
    -+  snprintf_(s, SIZEOF_ARRAY(s), fmt __VA_OPT__(,) __VA_ARGS__)
    ++  snprintf_(s, NITEMS(s), fmt __VA_OPT__(,) __VA_ARGS__)
      
      #define XSNPRINTF(s, fmt, ...)                                                 \
        xsnprintf(s, SIZEOF_ARRAY(s), fmt __VA_OPT__(,) __VA_ARGS__)
 9:  687285ab =  9:  2ffc9048 lib/: Use snprintf_() where appropriate
10:  dd9a7d93 = 10:  664ecbf5 tests/unit/test_snprintf.c: Test SNPRINTF()

@alejandro-colomar
Copy link
Collaborator Author

v3b changes:

  • Rebase to master
$ git range-diff master..664ecbf  shadow/master..snprintf 
 1:  8c6b3936 =  1:  b9d73d0b lib/, src/: snprintf(3) already terminates strings with NUL
 2:  61fb6205 =  2:  c939fbbc lib/sprintf.h: Add SNPRINTF() macro
 3:  178a764e =  3:  e799e849 src/login.c: Group preprocessor conditionals
 4:  da76b88f =  4:  786059ee lib/, src/: Align variable definitions
 5:  55587bcc =  5:  b4afad08 lib/, src/: Use SNPRINTF() instead of its pattern
 6:  9bece4b9 =  6:  c532e9ba lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  06134c94 =  7:  f5d4e664 lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  71725088 =  8:  55f3d724 lib/sprintf.[ch]: Add [v]snprintf_()
 9:  2ffc9048 =  9:  ab8f4168 lib/: Use snprintf_() where appropriate
10:  664ecbf5 = 10:  e6baaa76 tests/unit/test_snprintf.c: Test SNPRINTF()

@alejandro-colomar
Copy link
Collaborator Author

v4 changes:

  • Rebase to master.
  • Sort alphabetically file list in Makefile
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  b9d73d0b =  1:  5c1916dc lib/, src/: snprintf(3) already terminates strings with NUL
 2:  c939fbbc =  2:  4bc1cc49 lib/sprintf.h: Add SNPRINTF() macro
 3:  e799e849 =  3:  78f1721e src/login.c: Group preprocessor conditionals
 4:  786059ee =  4:  d5328791 lib/, src/: Align variable definitions
 5:  b4afad08 =  5:  845ccbbd lib/, src/: Use SNPRINTF() instead of its pattern
 6:  c532e9ba =  6:  d34d5a4c lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  f5d4e664 =  7:  81df5c9b lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  55f3d724 =  8:  30c1ff3e lib/sprintf.[ch]: Add [v]snprintf_()
 9:  ab8f4168 =  9:  a4c17f63 lib/: Use snprintf_() where appropriate
10:  e6baaa76 ! 10:  81c4b982 tests/unit/test_snprintf.c: Test SNPRINTF()
    @@ Commit message
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## tests/unit/Makefile.am ##
    -@@ tests/unit/Makefile.am: TESTS = $(check_PROGRAMS)
    +@@ tests/unit/Makefile.am: if HAVE_CMOCKA
    + TESTS = $(check_PROGRAMS)
      
      check_PROGRAMS = \
    -     test_strtcpy \
     +    test_snprintf \
    +     test_strncpy \
    +     test_strtcpy \
          test_xasprintf
    - 
    - if ENABLE_LOGIND
     @@ tests/unit/Makefile.am: test_strtcpy_LDADD = \
          $(CMOCKA_LIBS) \
          $(NULL)

@alejandro-colomar
Copy link
Collaborator Author

v5 changes:

  • Sort alphabetically
$ git range-diff shadow/master..gh/snprintf shadow/master..snprintf 
 1:  5c1916dc =  1:  5c1916dc lib/, src/: snprintf(3) already terminates strings with NUL
 2:  4bc1cc49 =  2:  4bc1cc49 lib/sprintf.h: Add SNPRINTF() macro
 3:  78f1721e =  3:  78f1721e src/login.c: Group preprocessor conditionals
 4:  d5328791 =  4:  d5328791 lib/, src/: Align variable definitions
 5:  845ccbbd =  5:  845ccbbd lib/, src/: Use SNPRINTF() instead of its pattern
 6:  d34d5a4c =  6:  d34d5a4c lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  81df5c9b =  7:  81df5c9b lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  30c1ff3e =  8:  30c1ff3e lib/sprintf.[ch]: Add [v]snprintf_()
 9:  a4c17f63 =  9:  a4c17f63 lib/: Use snprintf_() where appropriate
10:  81c4b982 ! 10:  e92c0370 tests/unit/test_snprintf.c: Test SNPRINTF()
    @@ tests/unit/Makefile.am: if HAVE_CMOCKA
          test_strncpy \
          test_strtcpy \
          test_xasprintf
    -@@ tests/unit/Makefile.am: test_strtcpy_LDADD = \
    -     $(CMOCKA_LIBS) \
    +@@ tests/unit/Makefile.am: test_logind_LDADD = \
    +     $(LIBSYSTEMD) \
          $(NULL)
      
     +test_snprintf_SOURCES = \
    @@ tests/unit/Makefile.am: test_strtcpy_LDADD = \
     +    $(CMOCKA_LIBS) \
     +    $(NULL)
     +
    - test_xasprintf_SOURCES = \
    -     ../../lib/sprintf.c \
    -     test_xasprintf.c \
    + test_strncpy_SOURCES = \
    +     test_strncpy.c \
    +     $(NULL)
     
      ## tests/unit/test_snprintf.c (new) ##
     @@

@alejandro-colomar
Copy link
Collaborator Author

v5b changes:

  • Rebase to master
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  5c1916dc =  1:  82583135 lib/, src/: snprintf(3) already terminates strings with NUL
 2:  4bc1cc49 =  2:  f7c1f331 lib/sprintf.h: Add SNPRINTF() macro
 3:  78f1721e =  3:  c21c86ea src/login.c: Group preprocessor conditionals
 4:  d5328791 =  4:  7056fd84 lib/, src/: Align variable definitions
 5:  845ccbbd =  5:  0c5bfb4d lib/, src/: Use SNPRINTF() instead of its pattern
 6:  d34d5a4c =  6:  1d03d317 lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  81df5c9b =  7:  6fa1ee0d lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  30c1ff3e =  8:  c12de50d lib/sprintf.[ch]: Add [v]snprintf_()
 9:  a4c17f63 =  9:  362dc304 lib/: Use snprintf_() where appropriate
10:  e92c0370 = 10:  0716befd tests/unit/test_snprintf.c: Test SNPRINTF()

@alejandro-colomar
Copy link
Collaborator Author

Queued after #854.

@alejandro-colomar alejandro-colomar marked this pull request as draft November 28, 2023 01:58
@alejandro-colomar
Copy link
Collaborator Author

v6 changes:

  • Rebase to master
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  82583135 =  1:  654e0133 lib/, src/: snprintf(3) already terminates strings with NUL
 2:  f7c1f331 =  2:  4ac045a8 lib/sprintf.h: Add SNPRINTF() macro
 3:  c21c86ea =  3:  1b8ee240 src/login.c: Group preprocessor conditionals
 4:  7056fd84 =  4:  4d235108 lib/, src/: Align variable definitions
 5:  0c5bfb4d =  5:  1133c03e lib/, src/: Use SNPRINTF() instead of its pattern
 6:  1d03d317 =  6:  fde280a9 lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  6fa1ee0d =  7:  0d901768 lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  c12de50d =  8:  e6c49014 lib/sprintf.[ch]: Add [v]snprintf_()
 9:  362dc304 =  9:  2a923abe lib/: Use snprintf_() where appropriate
10:  0716befd ! 10:  791873aa tests/unit/test_snprintf.c: Test SNPRINTF()
    @@ Commit message
         Signed-off-by: Alejandro Colomar <[email protected]>
     
      ## tests/unit/Makefile.am ##
    -@@ tests/unit/Makefile.am: if HAVE_CMOCKA
    - TESTS = $(check_PROGRAMS)
    +@@ tests/unit/Makefile.am: TESTS = $(check_PROGRAMS)
      
      check_PROGRAMS = \
    +     test_chkname \
     +    test_snprintf \
          test_strncpy \
          test_strtcpy \

@alejandro-colomar
Copy link
Collaborator Author

v7 changes:

  • Rebase to master
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  654e0133 =  1:  7fd54511 lib/, src/: snprintf(3) already terminates strings with NUL
 2:  4ac045a8 !  2:  2c4a14c2 lib/sprintf.h: Add SNPRINTF() macro
    @@ Commit message
     
      ## lib/sprintf.h ##
     @@
    - #include <stdio.h>
      
    + #include "attr.h"
      #include "defines.h"
     +#include "sizeof.h"
     +
 3:  1b8ee240 =  3:  b0c5f4cc src/login.c: Group preprocessor conditionals
 4:  4d235108 =  4:  9d2929ca lib/, src/: Align variable definitions
 5:  1133c03e !  5:  eed65e76 lib/, src/: Use SNPRINTF() instead of its pattern
    @@ lib/get_pid.c
     +
      int get_pid (const char *pidstr, pid_t *pid)
      {
    -   long long int val;
    +   long long  val;
     @@ lib/get_pid.c: int get_pidfd_from_fd(const char *pidfdstr)
      int open_pidfd(const char *pidstr)
      {
    @@ lib/subordinateio.c: static const struct subordinate_range *find_range(struct co
                      return NULL;
              }
              owner_uid = pwd->pw_uid;
    --        ret = snprintf(owner_uid_string, sizeof (owner_uid_string), "%lu", (unsigned long int)owner_uid);
    +-        ret = snprintf(owner_uid_string, sizeof (owner_uid_string), "%lu", (unsigned long)owner_uid);
     -        if (ret < 0 || (size_t)ret >= sizeof (owner_uid_string))
    -+        if (SNPRINTF(owner_uid_string, "%lu", (unsigned long int) owner_uid) == -1)
    ++        if (SNPRINTF(owner_uid_string, "%lu", (unsigned long) owner_uid) == -1)
                      return NULL;
      
              commonio_rewind(db);
 6:  fde280a9 =  6:  13c82132 lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  0d901768 =  7:  92ab39f4 lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  e6c49014 =  8:  b427cd32 lib/sprintf.[ch]: Add [v]snprintf_()
 9:  2a923abe =  9:  40e59cf0 lib/: Use snprintf_() where appropriate
10:  791873aa = 10:  b0bc924c tests/unit/test_snprintf.c: Test SNPRINTF()

@alejandro-colomar
Copy link
Collaborator Author

v8 changes:

  • Rebase to master (move to string/)
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  7fd54511 =  1:  82c5645c lib/, src/: snprintf(3) already terminates strings with NUL
 2:  2c4a14c2 !  2:  2f682d03 lib/sprintf.h: Add SNPRINTF() macro
    @@ Metadata
     Author: Alejandro Colomar <[email protected]>
     
      ## Commit message ##
    -    lib/sprintf.h: Add SNPRINTF() macro
    +    lib/string/sprintf.h: Add SNPRINTF() macro
     
         It wraps snprintf(3) so that it performs some steps that one might
         forget, or might be prone to accidents:
    @@ Commit message
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/sprintf.h ##
    + ## lib/string/sprintf.h ##
     @@
      
      #include "attr.h"
 3:  b0c5f4cc =  3:  fa24a0e9 src/login.c: Group preprocessor conditionals
 4:  9d2929ca =  4:  ef6a8f43 lib/, src/: Align variable definitions
 5:  eed65e76 !  5:  37b4cda7 lib/, src/: Use SNPRINTF() instead of its pattern
    @@ lib/commonio.c
      #include "prototypes.h"
      #include "commonio.h"
      #include "shadowlog_internal.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
      
      
      /* local function prototypes */
    @@ lib/get_pid.c
      #include <sys/stat.h>
      #include <fcntl.h>
      
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
     +
      int get_pid (const char *pidstr, pid_t *pid)
    @@ lib/hushed.c
      #include "defines.h"
      #include "prototypes.h"
      #include "getdef.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
     +
      /*
    @@ lib/nss.c
      #include "../libsubid/subid.h"
      #include "shadowlog_internal.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
      
      #define NSSWITCH "/etc/nsswitch.conf"
    @@ lib/pwauth.c
      #include "prototypes.h"
      #include "pwauth.h"
      #include "getdef.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
      
      #ifdef SKEY
      #include <skey.h>
    @@ lib/shell.c
      #include <errno.h>
      #include "prototypes.h"
      #include "defines.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
     +
      extern char **newenvp;
    @@ lib/subordinateio.c
      #include <fcntl.h>
      
      #include "alloc.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
      
      #define ID_SIZE 31
    @@ lib/user_busy.c
      #include "subordinateio.h"
      #endif                            /* ENABLE_SUBIDS */
      #include "shadowlog.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
      
      #ifdef __linux__
    @@ src/chage.c
      #include "pwio.h"
      #include "shadowio.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    - #include "strtcpy.h"
    ++#include "string/sprintf.h"
    + #include "string/strtcpy.h"
      #ifdef WITH_TCB
      #include "tcbfuncs.h"
     @@
    @@ src/chfn.c
      /*@-exitarg@*/
      #include "exitcodes.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    - #include "strtcpy.h"
    ++#include "string/sprintf.h"
    + #include "string/strtcpy.h"
      
     +
      /*
    @@ src/gpasswd.c
      /*@-exitarg@*/
      #include "exitcodes.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    - #include "strtcpy.h"
    ++#include "string/sprintf.h"
    + #include "string/strtcpy.h"
      
     +
      /*
    @@ src/login.c
      /*@-exitarg@*/
      #include "exitcodes.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    - #include "strtcpy.h"
    ++#include "string/sprintf.h"
    + #include "string/strtcpy.h"
      
      #ifdef USE_PAM
     @@ src/login.c: static /*@observer@*/const char *get_failent_user (/*@returned@*/const char *use
    @@ src/newgrp.c
      /*@-exitarg@*/
      #include "exitcodes.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
      
      /*
    @@ src/newusers.c
      #endif                            /* ENABLE_SUBIDS */
      #include "chkname.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
      
      /*
    @@ src/su.c
      /*@-exitarg@*/
      #include "exitcodes.h"
      #include "shadowlog.h"
    -+#include "sprintf.h"
    - #include "strtcpy.h"
    ++#include "string/sprintf.h"
    + #include "string/strtcpy.h"
      
     +
      /*
 6:  13c82132 !  6:  30c9e998 lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
    @@ Metadata
     Author: Alejandro Colomar <[email protected]>
     
      ## Commit message ##
    -    lib/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
    +    lib/string/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
     
         These wrappers are like [v]snprintf(), but exit on failure.  The macro
         calculates the size of the array internally (and guarantess that it is
    @@ Commit message
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/sprintf.c ##
    + ## lib/string/sprintf.c ##
     @@
      extern inline int xasprintf(char **restrict s, const char *restrict fmt, ...);
      extern inline int xvasprintf(char **restrict s, const char *restrict fmt,
    @@ lib/sprintf.c
     +extern inline int xvsnprintf(char *restrict s, int size,
     +    const char *restrict fmt, va_list ap);
     
    - ## lib/sprintf.h ##
    + ## lib/string/sprintf.h ##
     @@
        (len_ >= sz_) ? -1 : len_;                                            \
      })
    @@ lib/sprintf.h
      
      inline int
      xasprintf(char **restrict s, const char *restrict fmt, ...)
    -@@ lib/sprintf.h: xvasprintf(char **restrict s, const char *restrict fmt, va_list ap)
    +@@ lib/string/sprintf.h: xvasprintf(char **restrict s, const char *restrict fmt, va_list ap)
      }
      
      
 7:  92ab39f4 =  7:  139225d4 lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  b427cd32 !  8:  c724dada lib/sprintf.[ch]: Add [v]snprintf_()
    @@ Metadata
     Author: Alejandro Colomar <[email protected]>
     
      ## Commit message ##
    -    lib/sprintf.[ch]: Add [v]snprintf_()
    +    lib/string/sprintf.[ch]: Add [v]snprintf_()
     
         These functions are like [v]snprintf(3), but return -1 on truncation,
         which makes it easier to test.  snprintf(3) is iseful in two cases:
    @@ Commit message
     
         Signed-off-by: Alejandro Colomar <[email protected]>
     
    - ## lib/sprintf.c ##
    -@@ lib/sprintf.c: extern inline int xasprintf(char **restrict s, const char *restrict fmt, ...);
    + ## lib/string/sprintf.c ##
    +@@ lib/string/sprintf.c: extern inline int xasprintf(char **restrict s, const char *restrict fmt, ...);
      extern inline int xvasprintf(char **restrict s, const char *restrict fmt,
          va_list ap);
      
    @@ lib/sprintf.c: extern inline int xasprintf(char **restrict s, const char *restri
          const char *restrict fmt, ...);
      extern inline int xvsnprintf(char *restrict s, int size,
     
    - ## lib/sprintf.h ##
    + ## lib/string/sprintf.h ##
     @@
      
      
    @@ lib/sprintf.h
      
      #define XSNPRINTF(s, fmt, ...)                                                 \
        xsnprintf(s, SIZEOF_ARRAY(s), fmt __VA_OPT__(,) __VA_ARGS__)
    -@@ lib/sprintf.h: inline int xasprintf(char **restrict s, const char *restrict fmt, ...);
    +@@ lib/string/sprintf.h: inline int xasprintf(char **restrict s, const char *restrict fmt, ...);
      format_attr(printf, 2, 0)
      inline int xvasprintf(char **restrict s, const char *restrict fmt, va_list ap);
      
    @@ lib/sprintf.h: inline int xasprintf(char **restrict s, const char *restrict fmt,
      format_attr(printf, 3, 4)
      inline int xsnprintf(char *restrict s, int size, const char *restrict fmt, ...);
      format_attr(printf, 3, 0)
    -@@ lib/sprintf.h: xvasprintf(char **restrict s, const char *restrict fmt, va_list ap)
    +@@ lib/string/sprintf.h: xvasprintf(char **restrict s, const char *restrict fmt, va_list ap)
      }
      
      
    @@ lib/sprintf.h: xvasprintf(char **restrict s, const char *restrict fmt, va_list a
      inline int
      xsnprintf(char *restrict s, int size, const char *restrict fmt, ...)
      {
    -@@ lib/sprintf.h: xvsnprintf(char *restrict s, int size, const char *restrict fmt, va_list ap)
    +@@ lib/string/sprintf.h: xvsnprintf(char *restrict s, int size, const char *restrict fmt, va_list ap)
      {
        int  len;
      
 9:  40e59cf0 =  9:  3faad82c lib/: Use snprintf_() where appropriate
10:  b0bc924c ! 10:  2e214f3b tests/unit/test_snprintf.c: Test SNPRINTF()
    @@ tests/unit/Makefile.am: test_logind_LDADD = \
          $(NULL)
      
     +test_snprintf_SOURCES = \
    -+    ../../lib/sprintf.c \
    ++    ../../lib/string/sprintf.c \
     +    test_snprintf.c \
     +    $(NULL)
     +test_snprintf_CFLAGS = \
    @@ tests/unit/test_snprintf.c (new)
     +#include <cmocka.h>
     +
     +#include "sizeof.h"
    -+#include "sprintf.h"
    ++#include "string/sprintf.h"
     +
     +
     +static void test_SNPRINTF_trunc(void **state);

@alejandro-colomar alejandro-colomar marked this pull request as ready for review December 3, 2023 19:19
@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Dec 3, 2023 •
edited
Loading

If anyone thinks this has too many commits and prefers that I split it into several smaller PRs, feel free to suggest so. I kind of think it too.

@alejandro-colomar
Copy link
Collaborator Author

v8b changes:

  • Rebase to master
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  82c5645c =  1:  4f3ebb7a lib/, src/: snprintf(3) already terminates strings with NUL
 2:  2f682d03 =  2:  70846270 lib/string/sprintf.h: Add SNPRINTF() macro
 3:  fa24a0e9 =  3:  291aeb3a src/login.c: Group preprocessor conditionals
 4:  ef6a8f43 =  4:  adaab9e4 lib/, src/: Align variable definitions
 5:  37b4cda7 =  5:  343fd6d8 lib/, src/: Use SNPRINTF() instead of its pattern
 6:  30c9e998 =  6:  9caeaa46 lib/string/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  139225d4 =  7:  2b84863a lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  c724dada =  8:  ca368352 lib/string/sprintf.[ch]: Add [v]snprintf_()
 9:  3faad82c =  9:  0e8c50cb lib/: Use snprintf_() where appropriate
10:  2e214f3b = 10:  a72481e1 tests/unit/test_snprintf.c: Test SNPRINTF()

@alejandro-colomar
lib/, src/: snprintf(3) already terminates strings with NUL
6f24d5c 
We don't need to terminate them manually after the call.  Remove all
that paranoid code, which in some cases was even wrong.  While at it,
let's do a few more things:

-  Use sizeof(buf) for the size of the buffer.  I found that a few cases
   were passing one less byte (probably because the last one was
   manually zeroed later).  This caused a double NUL.  snprintf(3) wants
   the size of the entire buffer to properly terminate it.  Passing the
   exact value hardcoded is brittle, so use sizeof().

-  Align and improve style of variable declarations.  This makes them
   appear in this diff, which will help review the patch.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/string/sprintf.h: Add SNPRINTF() macro
635f891 
It wraps snprintf(3) so that it performs some steps that one might
forget, or might be prone to accidents:

-  It calculates the size of the destination buffer, and makes sure it's
   an array (otherwise, using sizeof(s) would be very bad).

-  It calculates if there's truncation or an error, returning -1 if so.

BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then the
static_assert(3) within NITEMS() makes sure VLAs are not allowed).

This macro is very similar to STRTCPY(), defined in lib/strtcpy.h.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
src/login.c: Group preprocessor conditionals
04873bd 
Group them at the end of the list of variable definitions, and use
'#if defined()' instead of '#if[n]def'.  Also indent nested ones.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/, src/: Align variable definitions
fe8ee5a 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/, src/: Use SNPRINTF() instead of its pattern
555759f 
The variable declarations for the buffers have been aligned in this
commit, so that they appear in the diff, making it easier to review.

Some important but somewhat tangent changes included in this commit:

-  lib/nss.c: The size was being defined as 65, but then used as 64.
   That was a bug, although not an important one; we were just wasting
   one byte.  Fix that while we replace snprintf() by SNPRINTF(), which
   will get the size from sizeof(), and thus will use the real size.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/string/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
b6fe227 
These wrappers are like [v]snprintf(), but exit on failure.  The macro
calculates the size of the array internally (and guarantess that it is
really an array), for added safety.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
58ecef0 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/string/sprintf.[ch]: Add [v]snprintf_()
bd47932 
These functions are like [v]snprintf(3), but return -1 on truncation,
which makes it easier to test.  snprintf(3) is iseful in two cases:

-  We don't care if the output is truncated.  snprintf(3) is fine for
   those, and the return value can be ignored.

-  Truncation is bad.  In that case, it's as bad as a hard error (-1)
   from snprintf, so merging both problems into the same error code
   makes it easier to handle errors.  Return the length if no truncation
   so that we can use it if necessary.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
lib/: Use snprintf_() where appropriate
511d1e4 
And where we don't need snprintf_(), remove the cast to (void), since
using snprintf(3) and not one of its wrappers necessarily means that the
return value is ignored.

By this, I'm not claiming that those calls to snprintf(3) without error
handling are correct; it's just that I'm not sure, so I leave them alone.

Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
tests/unit/test_snprintf.c: Test SNPRINTF()
42495a5 
Signed-off-by: Alejandro Colomar <[email protected]>
@alejandro-colomar
Copy link
Collaborator Author

v8c changes:

  • Rebase to master
$ git range-diff master..gh/snprintf shadow/master..snprintf 
 1:  4f3ebb7a =  1:  6f24d5c2 lib/, src/: snprintf(3) already terminates strings with NUL
 2:  70846270 =  2:  635f891c lib/string/sprintf.h: Add SNPRINTF() macro
 3:  291aeb3a =  3:  04873bd6 src/login.c: Group preprocessor conditionals
 4:  adaab9e4 =  4:  fe8ee5ab lib/, src/: Align variable definitions
 5:  343fd6d8 =  5:  555759ff lib/, src/: Use SNPRINTF() instead of its pattern
 6:  9caeaa46 =  6:  b6fe227e lib/string/sprintf.[ch]: Add x[v]snprintf() and XSNPRINTF()
 7:  2b84863a =  7:  58ecef0b lib/, src/: Use XSNPRINTF() instead of assert(SNPRINTF())
 8:  ca368352 =  8:  bd479327 lib/string/sprintf.[ch]: Add [v]snprintf_()
 9:  0e8c50cb =  9:  511d1e49 lib/: Use snprintf_() where appropriate
10:  a72481e1 = 10:  42495a52 tests/unit/test_snprintf.c: Test SNPRINTF()

@alejandro-colomar
Copy link
Collaborator Author

I'm splitting it into smaller PRs, starting at #864. I'll close this one.

@alejandro-colomar alejandro-colomar deleted the snprintf branch December 5, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@alejandro-colomar