Skip to content

Conversation

@clhin
Copy link
Contributor

@clhin clhin commented Nov 14, 2025

I was tired of staring at this every compile, since we check if the size is appropriate before we write, we can use asprintf and the compiler now trusts that we know what we are doing, and if we didn't we returned with -1 before it could allocate.

@WladimirBec
Copy link

Not sure if this is a good thing, asprintf will allocate some heap memory uselessly here, where the snprintf wouldn't.

Also your code is probably wrong:

strncpy(path, buf, sizeof(s.sun_path-1));
// should be:
strncpy(path, buf, sizeof(s.sun_path) - 1);

And sizeof(s.sun_path) - 1 was already assigned to the variable maxlen a little bit higher in the function, so no need to recalculate it:

strncpy(path, buf, maxlen);

@clhin
Copy link
Contributor Author

clhin commented Nov 15, 2025

Not sure if this is a good thing, asprintf will allocate some heap memory uselessly here, where the snprintf wouldn't.

This is unfortunately true, but I am not sure there is a better way of convincing the compiler we know what we are doing here other than simply silencing the warning.

Also your code is probably wrong:

strncpy(path, buf, sizeof(s.sun_path-1));
// should be:
strncpy(path, buf, sizeof(s.sun_path) - 1);

Fixed, thank you. The commit now passes all the cases and is ready for review.

And sizeof(s.sun_path) - 1 was already assigned to the variable maxlen a little bit higher in the function, so no need to recalculate it:

strncpy(path, buf, maxlen);

The compiler can optimize this into a constant instead of having to access memory which may take a number of cpu cycles, unless you make it constant.

@clhin
Copy link
Contributor Author

clhin commented Nov 16, 2025

Not sure if this is a good thing, asprintf will allocate some heap memory uselessly here, where the snprintf wouldn't.

I could try using strcat and then strcopy to get around this, but we will still need to make a buffer on the stack that has the same lifetime as the asprintf buffer...

@metux metux requested a review from a team November 17, 2025 11:31
@clhin
Copy link
Contributor Author

clhin commented Nov 20, 2025

If there are no further issues with this commit I would like it merged so that we can start thinking about setting -Wextra or -Werror

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