uuidgen: generate UUIDs in bounded batches to respect kernel limit#1965
uuidgen: generate UUIDs in bounded batches to respect kernel limit#1965NVSRahul wants to merge 1 commit intofreebsd:mainfrom
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thank you for taking the time to contribute to FreeBSD! All issues resolved. |
NVSRahul
force-pushed
the
uuidgen-batch-fix
branch
2 times, most recently
from
b406ddf to
6e48b12
Compare
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NVSRahul
force-pushed
the
uuidgen-batch-fix
branch
from
6e48b12 to
a00e249
Compare
|
Hello, All the CI checks are completed (or in progress for internal CI). Just double-checking if there’s anything else that I should adjust or work on regarding this change. Thanks for your time. |
NVSRahul
force-pushed
the
uuidgen-batch-fix
branch
2 times, most recently
from
ac95ee2 to
47696d8
Compare
bsdimp
left a comment
bsdimp
left a comment
There was a problem hiding this comment.
2048 being hard coded seems weird, but I don't see a #define or sysctl to get this value.
NVSRahul
force-pushed
the
uuidgen-batch-fix
branch
from
47696d8 to
9376f5f
Compare
|
Thanks for the review! I agree the hard-coded limit looks odd. Since the uuidgen(2) limit is enforced in the kernel and not exposed via a sysctl or public header, I’ve renamed the constant to UUIDGEN_BATCH_MAX and added a comment pointing directly to sys/kern/kern_uuid.c to document the origin and rationale. Please let me know if you’d prefer this handled differently. |
bin/uuidgen/uuidgen.c
Outdated
| * exposed via a sysctl or public header, mirror it here to | ||
| * avoid EINVAL and unbounded allocations. | ||
| */ | ||
| #define UUIDGEN_BATCH_MAX 2048 |
There was a problem hiding this comment.
At the risk of asking a silly question, why not instead add this to <sys/uuid.h> and use it both here and in kern_uuid.c? I would ditch your comment and steal the one from where it's used in sys_uuidgen (including the XXX, because maybe it should still be tunable)
bin/uuidgen/uuidgen.c
Outdated
| fprintf(fp, "%s\n", p); | ||
| free(p); | ||
| } | ||
| store = calloc(UUIDGEN_BATCH_MAX, sizeof(uuid_t)); |
There was a problem hiding this comment.
| store = calloc(UUIDGEN_BATCH_MAX, sizeof(uuid_t)); | |
| store = calloc(MIN(count, UUIDGEN_BATCH_MAX), sizeof(uuid_t)); |
Seems reasonable to clamp it down to count to avoid over-allocating, particularly in the common case where count=1.
NVSRahul
force-pushed
the
uuidgen-batch-fix
branch
from
9376f5f to
7c77fa0
Compare
|
Thanks! I've updated the calloc logic as suggested. I agree that moving the constant to <sys/uuid.h> is cleaner. I initially kept this PR scoped to userland, but I'm happy to include the header change here if you prefer. Should I go ahead and add that, or keep this PR limited to the binary? |
IMO it's a small enough change that it's fine to just do it here. If we wanted to MFC it independently then it would make sense to separate it, but we can confidently say that won't have much value in isolation if it hasn't happened by now. On that note also, one other nit: the manpage has a reference to 2048 that should probably just be removed if uuidgen(1) is going to smooth out the logistics now. |
NVSRahul
force-pushed
the
uuidgen-batch-fix
branch
from
7c77fa0 to
8adc67e
Compare
|
Done! I moved the constant to <sys/uuid.h>, updated both the kernel and uuidgen to use it, and cleaned up the manpage reference. |
| */ | ||
|
|
||
| #include <sys/capsicum.h> | ||
| #include <sys/param.h> |
There was a problem hiding this comment.
<sys/param.h> should come first (per style(9)), but we can just fix it pre-push unless another review tound is necessary.
The uuidgen(2) system call enforces a hard upper limit of 2048 UUIDs per invocation. uuidgen(1) previously attempted to generate arbitrary counts in a single call and allocated memory accordingly, leading to EINVAL errors, unnecessary memory usage, and potential overflow risks. Generate UUIDs in fixed-size batches, streaming output incrementally while preserving existing semantics. Mirror the kernel limit explicitly since it is not exposed via a public interface. Signed-off-by: NVSRahul <[email protected]>
NVSRahul
force-pushed
the
uuidgen-batch-fix
branch
from
8adc67e to
83948cf
Compare
3 participants
Fix uuidgen(1) to generate UUIDs in bounded batches that respect the
kernel uuidgen(2) limit.
The kernel uuidgen(2) system call is limited to 2048 UUIDs per call, but
uuidgen(1) attempted to generate arbitrarily many UUIDs in a single
invocation and allocate memory proportional to the request size. This
can lead to kernel errors, unnecessary memory usage, and potential
size_t overflow.
This change generates UUIDs in fixed-size batches, preserving streaming
semantics while respecting the kernel ABI and avoiding unbounded memory
allocation.