Bureaucracy & bikeshedding dose (fix a few off-by-one bugs)

[alejandro-colomar]

Cc: @hallyn , @ikerexxe , @stoeckmann

[alejandro-colomar]

v1b changes:

• Document some detail in the commit message.

$ git range-diff shadow/master gh/bureaucracy bureaucracy 
1:  23c7861f = 1:  23c7861f src/login.c: Fix off-by-one buggs
2:  fc929d47 = 2:  fc929d47 lib/: Don't say 'len' where 'size' is meant
3:  a8553c2c = 3:  a8553c2c src/login.c: Fix off-by-one bugss
4:  f767ca28 ! 4:  3236a9db lib/chkname.c: is_valid_user_name(): Remove unnecessary check
    @@ Commit message

         If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX).  And no size can
         ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports
    -    an unlimited user-name size via returning -1.
    +    an unlimited user-name size via returning -1.  Well, to be pedantic,
    +    that disallows a user-name siz of precisely SIZE_MAX bytes when
    +    sysconf(3) returns -1.  However, that's probably a good thing; such a
    +    long user name might trigger Undefined Behavior somewhere else, so be
    +    cautious and disallow it.  I hope nobody will be using the entire
    +    address space for a user name.

         The commit that introduced that check missed that this code had always
         supported unlimited user-name sizes since it was introduced by Iker in
    @@ Commit message
         What that patch did was adding code for handling EINVAL (or any other
         errors that a future kernel might add).

    +    To be more pedantically correct, that commit also allowed (under certain
    +    circumstances, user names of SIZE_MAX bytes, but those were originally
    +    allowed (by accident), and only became disallowed in 403a2e3771be
    +    ("lib/chkname.c: Take NUL byte into account").  But again, let's
    +    disallow those, just to be cautious.
    +
         Link: <https://github.com/shadow-maint/shadow/pull/935>
         Link: <https://github.com/shadow-maint/shadow/pull/935#discussion_r1477429492>
         See-also: 6be85b0bafb5 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning")
5:  9a129831 = 5:  8bdfaaa7 lib/chkname.c: is_valid_user_name(): Avoid a cast

[alejandro-colomar]

@hallyn , please consider this for 4.15

[alejandro-colomar]

v1c changes:

• Be more precise in commit message. size-1 != length. it's rather maxlen.

$ git range-diff shadow/master gh/bureaucracy bureaucracy 
1:  23c7861f ! 1:  0dea11e1 src/login.c: Fix off-by-one buggs
    @@ Commit message

                 login_prompt(username, max_size - 1);

    -    We're passing a length to functions that want a size.  But since the fix
    +    We're passing size-1 to functions that want a size.  But since the fix
         to those will be different, let's do that in the following commits.

         Link: <https://github.com/shadow-maint/shadow/pull/935>
2:  fc929d47 = 2:  19c19950 lib/: Don't say 'len' where 'size' is meant
3:  a8553c2c = 3:  3d4ea10b src/login.c: Fix off-by-one bugss
4:  3236a9db = 4:  71f56e39 lib/chkname.c: is_valid_user_name(): Remove unnecessary check
5:  8bdfaaa7 = 5:  c2210146 lib/chkname.c: is_valid_user_name(): Avoid a cast

[alejandro-colomar]

Hi Tobias,

On Mon, Feb 05, 2024 at 09:04:14AM -0800, Tobias Stoeckmann wrote:

@stoeckmann commented on this pull request.

errno = 0;

• maxlen = sysconf(_SC_LOGIN_NAME_MAX);
• if (maxlen == -1 && errno != 0)
• maxlen = LOGIN_NAME_MAX;
• if (maxlen != -1 && strlen(name) >= (size_t)maxlen)
• conf = sysconf(_SC_LOGIN_NAME_MAX);
• if (conf == -1 && errno != 0)
• maxsize = LOGIN_NAME_MAX;
• else
• maxsize = conf;

I'd suggest a (size_t) cast here, just to make clear that you are aware of the signed/unsigned difference here and that it's not a mistake.

I've never been aware enough of the problems of conversions that change sign, and have more than once written a bug in such a conversion, so I prefer not stating that it isn't a mistake, and let the commit message talk to future reviewers of the code. This way, the compiler can still remind us of the change, and if one enables -Weverything, it will trigger something here. Adding a cast would silence any warnings, especially if I did any mistakes, even when I think I haven't. And if in the future, someone finds that some pattern is dangerous and needs to be warned, having no cast will let the compiler do its job.

In general, I'm of the opinion that the right number of casts in an entire project is 0, and only casts within special macros like const_cast() are somehow acceptable (but still undesirable, and only necessary to address historic accidents like the fact that execve(3) doesn't get const strings).

Still, thanks for reviewing the patches, and for your proposal!

Have a lovely day, Alex

P.S.: I've now remembered that there's another place where a cast is acceptable: when the input is void *. That can hardly be made worse by a cast, and so a cast within the MALLOC() macros are actually good. Still, those casts are better hidden in a macro that makes sure those get few-or-none updates, and so the accidents with those tend to 0.

-- https://www.alejandro-colomar.es/ Looking for a remote C programming job at the moment.

[alejandro-colomar]

v1d changes:

• Rebase to master
• Add Reviewed-by tag [@ikerexxe ]

$ git range-diff master..gh/bureaucracy shadow/master..bureaucracy 
1:  0dea11e1 ! 1:  d636dbc7 src/login.c: Fix off-by-one buggs
    @@ Commit message
         Link: <https://github.com/shadow-maint/shadow/issues/674>
         See-also: 403a2e3771be ("lib/chkname.c: Take NUL byte into account")
         Fixes: 3b7cc053872c ("lib: replace `USER_NAME_MAX_LENGTH` macro")
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
         Cc: Serge Hallyn <serge@hallyn.com>
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/login.c ##
2:  19c19950 ! 2:  855e3f2e lib/: Don't say 'len' where 'size' is meant
    @@ Commit message
         See-also: 403a2e3771be ("lib/chkname.c: Take NUL byte into account")
         See-also: 6a1f45d932c8 ("lib/chkname.c: Support unlimited user name lengths")
         Fixes: 95ea61009da8 ("lib/chkname.c: Use precise comment")
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
         Cc: Serge Hallyn <serge@hallyn.com>
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## lib/chkname.c ##
3:  3d4ea10b ! 3:  d893a0d6 src/login.c: Fix off-by-one bugss
    @@ Commit message
         See-also: 0656a90bfd0d ("src/login.c: Fix off-by-one buggs")
         See-also: 403a2e3771be ("lib/chkname.c: Take NUL byte into account")
         Fixes: 3b7cc053872c ("lib: replace `USER_NAME_MAX_LENGTH` macro")
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
         Cc: Serge Hallyn <serge@hallyn.com>
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/login.c ##
4:  71f56e39 ! 4:  12fd1121 lib/chkname.c: is_valid_user_name(): Remove unnecessary check
    @@ Commit message
         Link: <https://github.com/shadow-maint/shadow/pull/935#discussion_r1477429492>
         See-also: 6be85b0bafb5 ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning")
         Fixes: 6a1f45d932c8 ("lib/chkname.c: Support unlimited user name lengths")
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
         Cc: Serge Hallyn <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
5:  c2210146 ! 5:  e5ce948a lib/chkname.c: is_valid_user_name(): Avoid a cast
    @@ Commit message

         By using a temporary vairable, we can remove a cast.

    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
         Cc: Serge Hallyn <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

[alejandro-colomar]

LGTM! Still, I'd prefer a second pair of eyes to take a look before merging.

So, let's wait for @hallyn .