Skip to content

sfdisk: fix memory leak and related to using invalid memoryin sfdisk_print_partition()#4459

Open
Leefancy wants to merge 1 commit into
util-linux:masterfrom
Leefancy:branch-01
Open

sfdisk: fix memory leak and related to using invalid memoryin sfdisk_print_partition()#4459
Leefancy wants to merge 1 commit into
util-linux:masterfrom
Leefancy:branch-01

Conversation

@Leefancy

Copy link
Copy Markdown

The function fdisk_partition_to_string() dynamically allocates memory
for the returned string. In sfdisk_print_partition(), this function is
called multiple times in succession to print various partition fields
(DEVICE, START, END, SIZE, and TYPE).

Comment thread disk-utils/sfdisk.c
if (0 == rc) {
printf("%12s : ", data);
free(data);
}

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.

The function fdisk_partition_to_string() keeps &data NULL on error, so you can use:

if (fdisk_partition_to_string(pa, sf->cxt, FDISK_FIELD_DEVICE, &data) == 0)
    printf("%12s : ", data);
free(data);

Comment thread disk-utils/sfdisk.c Outdated
printf("(%s) ", data);
rc = fdisk_partition_to_string(pa, sf->cxt, FDISK_FIELD_SIZE, &data);
if (0 == rc) {
printf("%12s : ", data);

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, be careful with copy & paste, it's "(%s) ".

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have done it.

Comment thread disk-utils/sfdisk.c Outdated
printf("%s\n", data);
rc = fdisk_partition_to_string(pa, sf->cxt, FDISK_FIELD_TYPE, &data);
if (0 == rc) {
printf("%12s : ", data);

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.

"%s\n"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have done it.

…print_partition()

Signed-off-by: lijian <lijian01@kylinos.cn>
@Leefancy

Leefancy commented Jul 2, 2026

Copy link
Copy Markdown
Author

Thanks for the review feedback. I don't think we can simplify the code this way, for two reasons:
0) data is not guaranteed to be NULL on failure. fdisk_partition_to_string() does not necessarily set the data pointer to NULL when its execution fails, so the free() operation must be kept inside the if block to avoid invalid memory deallocation.

  1. Since this function is called multiple times in sequence, if one call to fdisk_partition_to_string() succeeds while another fails, placing free() outside the if statement would still be dangerous. The failed invocation may leave data with an invalid value instead of NULL, which would result in an unsafe free operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants