Skip to content
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.

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

Feature/remove sed runtime dependency #683

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

sabedevops
Copy link
Contributor

No description provided.

@sabedevops sabedevops requested a review from a team as a code owner June 29, 2023 17:28
bool copy_r = make_copy(RESOLV_CONF_FILE, RESOLV_CONF_FILE ".bkp");

const char *match = "nameserver ";
off_t replace_size = strlen(match) + strlen(addr) + sizeof(char);
Copy link
Member

Choose a reason for hiding this comment

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

We should add 2 * sizeof(char) since since the replace string now includes a newline (in addition to the terminating nul).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is handled in #L466. Accounting for the null byte is always handled in the allocation functions in this code so that I could keep it consistent across malloc and realloc when computing the realloc size in #L533. In other words, *_size here indicates the size without the null bytes.

strcpy(replace, match);
strcat(replace, addr);
strcat(replace, "\n");
Copy link
Member

Choose a reason for hiding this comment

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

strncat would help to prevent overruns here.

building the replace string with a single call to snprintf might be cleaner.

}
remaining_content[remaining_size] = '\0';

_cleanup_(cleanup_bufferp) char *rptr = realloc(replace, (size_t)(replace_size + remaining_size + 1));
Copy link
Member

Choose a reason for hiding this comment

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

why use a new variable here? I thought the common realloc pattern was p = realloc(p, size); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had some doubts about correct usage here after careful reading of the man page (it has very quirky behavior), so I took inspiration from information shared here:

https://stackoverflow.com/questions/44789295/correct-use-of-realloc

Additionally, there is interplay with the cleanup() function guard here I'm being careful to consider so we don't double free() in the case where the allocated memory block is moved from it's original location since realloc() will free() it internally. See #L534-544

@sabedevops sabedevops merged commit c1c117c into main Jul 12, 2023
25 checks passed
@sabedevops sabedevops deleted the feature/remove-sed-runtime-dependency branch July 12, 2023 14:34
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.

None yet

2 participants