Skip to content

Conversation

@agialluc
Copy link

Updated conserver/cutil.c and conserver/group.c to use snprintf( ) vice sprintf( ) to prevent buffer overflows.

This code change should be carefully checked. I am not normally a coder.

…ce sprintf( ) to prevent buffer overflows.

Signed-off-by: Anthony Gialluca <agialluc@redhat.com>
{
char buf[1024];
sprintf(buf, "CONSFILE-%s-%lu-%d.w", progname,
snprintf(buf, 1024, "CONSFILE-%s-%lu-%d.w", progname,
Copy link

@fweimer-rh fweimer-rh Sep 26, 2023

Choose a reason for hiding this comment

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

Suggested change
snprintf(buf, 1024, "CONSFILE-%s-%lu-%d.w", progname,
snprintf(buf, sizeof(buf), "CONSFILE-%s-%lu-%d.w", progname,

But if things are crashing here, we know that buf isn't large enough. Given what is written here (and below), that seems unlikely? Probably something else is going on.

Copy link
Author

Choose a reason for hiding this comment

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

This is an excellent recommendation. It makes it much cleaner. With respect to what size buf should be, I am open to what is recommended.

return telopts[o];
else {
sprintf(opt, "%d", o);
snprintf(opt, 128, "%d", o);

Choose a reason for hiding this comment

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

Suggested change
snprintf(opt, 128, "%d", o);
snprintf(opt, sizeof(opt), "%d", o);

Copy link
Author

Choose a reason for hiding this comment

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

Again I very much agree with what you are doing, this is an excellent recommendation.

….c with recommended code change.

Signed-off-by: Anthony Gialluca <agialluc@redhat.com>
@agialluc
Copy link
Author

I should note, for the record, this may not be the root cause, but is just my suspicion. Regardless, I do think snprintf( ) calls are an improvement.

Copy link
Contributor

@robohack robohack left a comment

Choose a reason for hiding this comment

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

I don't think these changes will solve any crash -- in cutil.c they are in debug code and anyway the supplied buffer should always be much bigger than any possible expansion of the format string; and in group.c an integer will never be printed as more than 127 decimal digits (unless ints are 420 bits or more).

@agialluc
Copy link
Author

I was a beautiful theory while it lasted. Please feel free to abandon this merge as necessary.

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.

3 participants