subids: Some cleanups

[alejandro-colomar]

Cc: @jcpunk

I'm preparing some cleanups for you to pick in your PR.

---

Revisions:

v2

• Prevent wrap-around by moving +1 to the other side of the comparison. [@hallyn]

$ git range-diff 5f628ddc^ gh/subids subids 
1:  5f628ddc = 1:  5f628ddc lib/: find_new_sub_*ids(): Remove dead assertions
2:  3b3b1ee1 = 2:  3b3b1ee1 lib/, src/: find_new_sub_[ug]ids(): Report errors through errno
3:  1309fafa = 3:  1309fafa autogen.sh: CFLAGS: Remove -Werror=sign-compare
4:  7e8c3b71 = 4:  7e8c3b71 autogen.sh: CFLAGS: Add -Werror=overflow
5:  cffaa562 = 5:  cffaa562 lib/subordinateio.c: find_free_range(): Use id_t instead of u_long
6:  d65eed7e = 6:  d65eed7e lib/: find_free_range(): Set errno or error
7:  3f0338c8 ! 7:  2bab0630 lib/subordinateio.c: find_free_range(): Validate input more carefully
    @@ Metadata
      ## Commit message ##
         lib/subordinateio.c: find_free_range(): Validate input more carefully
     
    +    Cc: Serge Hallyn <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/subordinateio.c ##
    @@ lib/subordinateio.c: find_free_range(struct commonio_db *db, id_t min, id_t max,
      
     -  /* When given invalid parameters fail */
     -  if ((count == 0) || (max < min)) {
    -+  if (count == 0 || max < min || count > max - min + 1) {
    ++  if (count == 0 || max < min || count - 1 > max - min) {
                errno = ERANGE;
                return -1;
        }
8:  9c4a8561 = 8:  126be682 lib/: find_new_sub_[ug]ids(): Remove redundant checks

v2b

• Rebase

$ git rd 
1:  5f628ddc ! 1:  4596af19 lib/: find_new_sub_*ids(): Remove dead assertions
    @@ Commit message
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/find_new_sub_gids.c ##
    -@@
    - 
    - #ifdef ENABLE_SUBIDS
    - 
    --#include <assert.h>
    - #include <stdio.h>
    - #include <errno.h>
    - 
     @@ lib/find_new_sub_gids.c: int find_new_sub_gids (id_t *range_start, unsigned long *range_count)
        unsigned long count;
        id_t start;
    @@ lib/find_new_sub_gids.c: int find_new_sub_gids (id_t *range_start, unsigned long
        count = getdef_ulong ("SUB_GID_COUNT", 65536);
     
      ## lib/find_new_sub_uids.c ##
    -@@
    - 
    - #ifdef ENABLE_SUBIDS
    - 
    --#include <assert.h>
    - #include <stdio.h>
    - #include <errno.h>
    - 
     @@ lib/find_new_sub_uids.c: int find_new_sub_uids (id_t *range_start, unsigned long *range_count)
        unsigned long count;
        id_t start;
2:  3b3b1ee1 = 2:  4b3001ff lib/, src/: find_new_sub_[ug]ids(): Report errors through errno
3:  1309fafa = 3:  bfedf40e autogen.sh: CFLAGS: Remove -Werror=sign-compare
4:  7e8c3b71 = 4:  aa96fb51 autogen.sh: CFLAGS: Add -Werror=overflow
5:  cffaa562 = 5:  f9b0a46e lib/subordinateio.c: find_free_range(): Use id_t instead of u_long
6:  d65eed7e = 6:  3780bc2a lib/: find_free_range(): Set errno or error
7:  2bab0630 = 7:  ecdd0aa6 lib/subordinateio.c: find_free_range(): Validate input more carefully
8:  126be682 = 8:  b6a0a065 lib/: find_new_sub_[ug]ids(): Remove redundant checks

v2c

• Rebase
• tfix

$ git rd 
1:  4596af19 = 1:  55120544 lib/: find_new_sub_*ids(): Remove dead assertions
2:  4b3001ff = 2:  7f6086ca lib/, src/: find_new_sub_[ug]ids(): Report errors through errno
3:  bfedf40e = 3:  9514b435 autogen.sh: CFLAGS: Remove -Werror=sign-compare
4:  aa96fb51 = 4:  d1b29220 autogen.sh: CFLAGS: Add -Werror=overflow
5:  f9b0a46e = 5:  82568848 lib/subordinateio.c: find_free_range(): Use id_t instead of u_long
6:  3780bc2a ! 6:  f6086011 lib/: find_free_range(): Set errno or error
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    lib/: find_free_range(): Set errno or error
    +    lib/: find_free_range(): Set errno on error
     
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
7:  ecdd0aa6 = 7:  a6f155a2 lib/subordinateio.c: find_free_range(): Validate input more carefully
8:  b6a0a065 = 8:  2b405be5 lib/: find_new_sub_[ug]ids(): Remove redundant checks

v2d

• Rebase

$ git rd 
1:  55120544 = 1:  c1bd6b2a lib/: find_new_sub_*ids(): Remove dead assertions
2:  7f6086ca = 2:  19f8d02f lib/, src/: find_new_sub_[ug]ids(): Report errors through errno
3:  9514b435 = 3:  9ce93402 autogen.sh: CFLAGS: Remove -Werror=sign-compare
4:  d1b29220 = 4:  2eb59985 autogen.sh: CFLAGS: Add -Werror=overflow
5:  82568848 = 5:  d939a079 lib/subordinateio.c: find_free_range(): Use id_t instead of u_long
6:  f6086011 = 6:  6d9cc42d lib/: find_free_range(): Set errno on error
7:  a6f155a2 = 7:  3887674c lib/subordinateio.c: find_free_range(): Validate input more carefully
8:  2b405be5 = 8:  775435e0 lib/: find_new_sub_[ug]ids(): Remove redundant checks

v2e

• Expand commit message.

$ git rd 
1:  c1bd6b2a = 1:  c1bd6b2a lib/: find_new_sub_*ids(): Remove dead assertions
2:  19f8d02f = 2:  19f8d02f lib/, src/: find_new_sub_[ug]ids(): Report errors through errno
3:  9ce93402 = 3:  9ce93402 autogen.sh: CFLAGS: Remove -Werror=sign-compare
4:  2eb59985 = 4:  2eb59985 autogen.sh: CFLAGS: Add -Werror=overflow
5:  d939a079 = 5:  d939a079 lib/subordinateio.c: find_free_range(): Use id_t instead of u_long
6:  6d9cc42d = 6:  6d9cc42d lib/: find_free_range(): Set errno on error
7:  3887674c ! 7:  70f0214d lib/subordinateio.c: find_free_range(): Validate input more carefully
    @@ Metadata
      ## Commit message ##
         lib/subordinateio.c: find_free_range(): Validate input more carefully
     
    +    The test is
    +            if (count == 0 || max < min || count - 1 > max - min) {
    +
    +    We tested here a couple of obvious things
    +    -  the count is nonzero.
    +    -  min >= max
    +
    +    But we should also make sure that the count fits within the range
    +    [min, max].  For that, we must be very careful, to avoid unsigned
    +    integer wrap-around.  max-min can't wrap, since we already tested that
    +    max>=min.  And count-1 can't wrap, since count!=0.
    +
    +    A previous version of this patch wrote it as
    +            ... || count > max - min + 1)
    +    which is bogus.  When max==UINT_MAX, and min=0, then max-min==UINT_MAX,
    +    and when doing +1, we wrap again to 0.  Unsigned integer types are very
    +    dangerous, and should be handled very carefully.
    +
         Cc: Serge Hallyn <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
8:  775435e0 = 8:  7a26be1f lib/: find_new_sub_[ug]ids(): Remove redundant checks

[jcpunk]

I'm showing:

subordinateio.c: In function 'new_subid_range':
subordinateio.c:1077:58: warning: conversion from 'long unsigned int' to 'id_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Woverflow]
 1077 |         range->start = find_free_range(db, range->start, ULONG_MAX, range->count);
      |                                                          ^~~~~~~~~
subordinateio.c:1078:26: warning: comparison of integer expressions of different signedness: 'long unsigned int' and 'int' [-Wsign-compare]
 1078 |         if (range->start == -1) {
      |                          ^~

which are probably issues?

[alejandro-colomar]

I'm showing:

subordinateio.c: In function 'new_subid_range':
subordinateio.c:1077:58: warning: conversion from 'long unsigned int' to 'id_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Woverflow]
 1077 |         range->start = find_free_range(db, range->start, ULONG_MAX, range->count);
      |                                                          ^~~~~~~~~
subordinateio.c:1078:26: warning: comparison of integer expressions of different signedness: 'long unsigned int' and 'int' [-Wsign-compare]
 1078 |         if (range->start == -1) {
      |                          ^~

which are probably issues?

Yeah, I need to fix some issues I've seen in CI. I'll reply when they seem fixed. Thanks!

This warning probably explains some of the CI errors. I'll check if we can enable -Werror=overflow globally.

[alejandro-colomar]

@jcpunk Please pick the first 6 commits for your PR. Those are passing CI, and locally I verified they don't have any bad warnings (there are some warnings, but they are false positives).

The last two commits are problematic, so don't pick them.

[jcpunk]

I'll get to those in the morning. Is there a reason to remove the sign compare warning? I feel like that would help identify oddities...

[alejandro-colomar]

I'll get to those in the morning.

Ok.

Is there a reason to remove the sign compare warning? I feel like that would help identify oddities...

Yes, see this GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119011.

It's a nice diagnostic, if it didn't diagnose literal -1. Until GCC fixes that, we can't use it.

[hallyn]

* I've fixed the -Woverflow diagnostic, but it wasn't the issue I was experiencing.

...

The patch is

commit 3f0338c854227eeea385ad4ade5985cec8f08198 (HEAD -> subids, gh/subids)
Author: Alejandro Colomar <alx@kernel.org>
Date:   2026-03-15 15:55:30 +0100

    lib/subordinateio.c: find_free_range(): Validate input more carefully
    
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/lib/subordinateio.c b/lib/subordinateio.c
index 6fa44f3a..15a03c89 100644
--- a/lib/subordinateio.c
+++ b/lib/subordinateio.c
@@ -352,8 +352,7 @@ find_free_range(struct commonio_db *db, id_t min, id_t max, unsigned long count)
        id_t                           low, high;
        const struct subordinate_range *range;
 
-       /* When given invalid parameters fail */
-       if ((count == 0) || (max < min)) {
+       if (count == 0 || max < min || count > max - min + 1) {
                errno = ERANGE;
                return -1;
        }

I'm not sure what you mean by you've addressed the overflow issue. Your patches
makes min and max id_t, and count unsigned long. When I do that, and pass in the
values that new_subid_range passes in, I get:

serge@sl25:~/test$ cat ids.c
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>

int main() {
	id_t min = 0;
	id_t max = (id_t) ULONG_MAX;
	unsigned long count;
	
	if (500 > max  - min + 1) printf("yup\n");
}
serge@sl25:~/test$ gcc -o ids ids.c
serge@sl25:~/test$ ./ids
yup

whereas when min and max are unsigned long, the test returns false.

[alejandro-colomar]

* I've fixed the -Woverflow diagnostic, but it wasn't the issue I was experiencing.

[...]

I'm not sure what you mean by you've addressed the overflow issue.

That was an unrelated issue.

Your patches makes min and max id_t, and count unsigned long. When I do that, and pass in the values that new_subid_range passes in, I get:

serge@sl25:~/test$ cat ids.c
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>

int main() {
	id_t min = 0;
	id_t max = (id_t) ULONG_MAX;
	unsigned long count;
	
	if (500 > max  - min + 1) printf("yup\n");
}
serge@sl25:~/test$ gcc -o ids ids.c
serge@sl25:~/test$ ./ids
yup

whereas when min and max are unsigned long, the test returns false.

I see it being true with both id_t and u_long:

alx@devuan:~/tmp$ cat u.c 
#include <limits.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

#ifdef FOO
typedef id_t  foo_t;
#else
typedef unsigned long  foo_t;
#endif

int
main(void)
{
	foo_t min = 0;
	foo_t max = (foo_t) ULONG_MAX;

	printf("min: %ju\n", (uintmax_t) min);
	printf("max: %ju\n", (uintmax_t) min);
	printf("max - min: %ju\n", (uintmax_t) (max - min));
	printf("max - min + 1: %ju\n", (uintmax_t) (max - min + 1));

	if (500 > max  - min + 1) printf("yup\n");
}

alx@devuan:~/tmp$ gcc -Wall -Wextra u.c 
alx@devuan:~/tmp$ ./a.out 
min: 0
max: 0
max - min: 18446744073709551615
max - min + 1: 0
yup
alx@devuan:~/tmp$ gcc -Wall -Wextra u.c -DFOO
alx@devuan:~/tmp$ ./a.out 
min: 0
max: 0
max - min: 4294967295
max - min + 1: 0
yup

But your program allowed me to find the issue: I should move the +1 to the other side of the comparison.

count - 1 > max - min

max - min will never wrap around, since we know max > min.
And count - 1 will also never wrap around, since we know count != 0.

Thanks!

[alejandro-colomar]

@hallyn

This has fixed CI. Thanks!

@jcpunk

Now you can pick all commits to your PR.

[jcpunk]

I think I've got that done

[hallyn]

max - min will never wrap around, since we know max > min.
And count - 1 will also never wrap around, since we know count != 0.

Haaahaha :) that commit makes me laugh :) That's the kind of thing automation certainly ought to be able to warn us about/remind us of.

Thanks.

[alejandro-colomar]

max - min will never wrap around, since we know max > min.
And count - 1 will also never wrap around, since we know count != 0.

Haaahaha :) that commit makes me laugh :) That's the kind of thing automation certainly ought to be able to warn us about/remind us of.

Agree. Although it won't be easy. It would trigger too many false positives.

That's the reason why unsigned types are much more dangerous than signed ones, and the bad fame that signed integer overflow has in C is unmerited. At least, compilers have become quite good at diagnostic signed integer overflow.

Unsigned integer wrap-around, while well-defined, is way more dangerous, precisely because by being well-defined, the compiler must assume that you did that on purpose (i.e., you wanted the wrap-around behavior).

There's some work at the moment, with attributes, to differentiate between unsigned integers where you want the wrap-around, vs the ones where you want a diagnostic. @kees talked about this last summer in LSS in Denver. @uecker may also know about it.

Thanks.

You're welcome! I'll update the commit to document the issue better.

[alejandro-colomar]

max - min will never wrap around, since we know max > min.
And count - 1 will also never wrap around, since we know count != 0.

Haaahaha :) that commit makes me laugh :) That's the kind of thing automation certainly ought to be able to warn us about/remind us of.

We're lucky that we had tests. :)

Agree. Although it won't be easy. It would trigger too many false positives.

That's the reason why unsigned types are much more dangerous than signed ones, and the bad fame that signed integer overflow has in C is unmerited. At least, compilers have become quite good at diagnostic signed integer overflow.

Unsigned integer wrap-around, while well-defined, is way more dangerous, precisely because by being well-defined, the compiler must assume that you did that on purpose (i.e., you wanted the wrap-around behavior).

There's some work at the moment, with attributes, to differentiate between unsigned integers where you want the wrap-around, vs the ones where you want a diagnostic. @kees talked about this last summer in LSS in Denver. @uecker may also know about it.

Thanks.

You're welcome! I'll update the commit to document the issue better.

[uecker]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116193