Skip to content

Fix bug from de7f1c78f70f8df4b2956f7363da3774348bc58e that caused security regression #1630

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

Open
MillaFleurs wants to merge 2 commits into
base: master
Choose a base branch
from
+36 −19
Open

Fix bug from de7f1c78f70f8df4b2956f7363da3774348bc58e that caused security regression #1630

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0de9eea
Fix bug from de7f1c78f70f8df4b2956f7363da3774348bc58e that changed fr…
KD2YCU May 23, 2026
d25d136
lib: use signed arithmetic in find_free_range()
KD2YCU May 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 36 additions & 19 deletions lib/subordinateio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <pwd.h>
#include <ctype.h>
#include <fcntl.h>
#include <stdint.h>
#include <string.h>

#include "alloc/malloc.h"
Expand Down Expand Up @@ -332,10 +333,12 @@ static int subordinate_range_cmp (const void *p1, const void *p2)
static id_t
find_free_range(struct commonio_db *db, id_t min, id_t max, unsigned long count)
{

@alejandro-colomar alejandro-colomar May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're relying on the fact that intmax_t is strictly wider than id_t, we could declare that in a static assertion (just to preclude any hypothetical future systems where id_t might be 64-bit).

How about adding this at the top of this function?

static_assert(sizeof(intmax_t) > sizeof(id_t), "");

id_t low, high;
intmax_t low, max_id;
const struct subordinate_range *range;

if (count == 0 || max < min || count - 1 > max - min) {
if (count == 0 || max < min
|| (uintmax_t) count - 1 > (uintmax_t) max - min)
{
Comment on lines -338 to +341

@alejandro-colomar alejandro-colomar May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also make these casts be signed.

I think the following should be okay:

if (count == 0 || max < min
 || (intmax_t) count - 1 > (intmax_t) max - min)
{

Also, I think we can avoid one cast if we re-organize the comparison:

if (count == 0 || max < min || (intmax_t) count > max - min + 1LL) {

Because both max and min are id_t, and max >= min, their subtraction is in the range 0 .. (id_t)-1, and adding 1LL to that is guaranteed to fit in long long. Because long long is wider than id_t, this is going to be signed. (And we also make the conditional fit in one line.)

errno = ERANGE;
return -1;
}
Expand All @@ -345,33 +348,48 @@ find_free_range(struct commonio_db *db, id_t min, id_t max, unsigned long count)
commonio_rewind(db);

low = min;
max_id = max;
while (NULL != (range = commonio_next(db))) {
id_t first, last;
intmax_t first, last;

first = range->start;
last = first + range->count - 1;

/* Find the top end of the hole before this range */
high = first;
if (range->count == 0)
continue;

/* Don't allocate IDs after max (included) */
if (high > max + 1) {
high = max + 1;
first = range->start;
if (__builtin_add_overflow(first, range->count - 1, &last))
last = INTMAX_MAX;

Comment on lines 352 to +361

@alejandro-colomar alejandro-colomar May 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary to fix the regression?

This arithmetic seems to be done on unsigned long, and hasn't changed to id_t in the commit that introduced the regression, AFAICS.

I'm not saying this change is not okay, but I think that's fixing a separate problem, and that should be done in a separate commit whose commit message documents clearly.

Or am I missing some reason why this would be necessary to fix the regression?

@alejandro-colomar alejandro-colomar May 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like the commit that fixes the regression to be minimal.

/*
* Ranges are sorted by start, and low has been advanced past
* every range seen so far, so the gap [low, first - 1] (clamped
* to max) contains no existing range. A block that fits in
* this gap lies below first, hence below every later range too,
* so it cannot overlap any allocation.
*/
if (first > low) {
intmax_t high = first - 1;
Comment on lines +369 to +370

@alejandro-colomar alejandro-colomar May 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please separate declarations from code:

if (first > low) {
	intmax_t  high;

	high = first - 1;


/* Don't allocate IDs after max (included). */
if (high > max_id)
high = max_id;

/* Is the hole before this range large enough? */
if ((high >= low) && ((high - low + 1) >= (intmax_t) count))
return low;
}

/* Is the hole before this range large enough? */
if ((high > low) && ((high - low) >= count))
return low;

/* Compute the low end of the next hole */
if (low < (last + 1))
if (last >= low) {
if (last == INTMAX_MAX)
goto fail;
low = last + 1;
if (low > max)
}
if (low > max_id)
goto fail;
}
Comment on lines 367 to 389

@alejandro-colomar alejandro-colomar May 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that INTMAX_MAX is necessarily going to be >= low, we can flatten that code:

if (last == INTMAX_MAX)
	goto fail;
if (last >= low)
	low = last + 1;
if (low > max_id)
	goto fail;

However, is it really possible for last to be INTMAX_MAX? Or can we entirely remove that branch?


/* Is the remaining unclaimed area large enough? */
if (((max - low) + 1) >= count)
if (((max_id - low) + 1) >= (intmax_t) count)

@alejandro-colomar alejandro-colomar May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make the count parameter be of type long long to avoid these casts?

return low;
fail:
errno = EUSERS;
Expand Down Expand Up @@ -1128,4 +1146,3 @@ void free_subid_pointer(void *ptr)
#else /* !ENABLE_SUBIDS */
extern int ISO_C_forbids_an_empty_translation_unit;
#endif /* !ENABLE_SUBIDS */

Loading