i#7274: Warn on possible strncpy truncation in func_trace.cpp

[ashfordium]

Issue #7274

Warn on possible strncpy truncation in func_trace.cpp, avoid null byte copies

[ashfordium]

Sorry for force push, I had the wrong git config on the commit, adding someone else to this PR on accident...

[ashfordium]

This is also technically a performance improvement. strncpy always writes dsize bytes. edit: Remove snippet about gnu libc since I see the redefines in core/globals.h and core/string.c

The above patch avoids always writing 2048 bytes for each function metadata symbol created, and the memcpy avoids a second strlen that would happen if this was changed to strncpy(f->name, name, name_len + 1).

[edeiana]

Thank you for contributing!

We usually start by filing an issue.

I have done it for you in this case: #7274.

Please add answers to the following questions in that issue:

• What version of DynamoRIO are you using?
• Does the latest build from https://github.com/DynamoRIO/dynamorio/releases solve the problem?
• What operating system version are you running on? ("Windows 10" is not sufficient: give the release number.)
• Is your application 32-bit or 64-bit?

And add a snippet of the errors during build.

Please edit your PR description to remove the redundant information now in #7274 and only describe your code changes and their effect.

Also, please change the PR title adding the issue number first:

i#7274: Fix strncpy truncation error from gcc stringop-truncations

You also don't need to create multiple single-commit PRs (#7268 #7269).
Since all those changes are related to #7274 you can put them in a single PR, just add-commit-push to the same branch.
And, as you noted, please avoid force-push.

[ashfordium]

Even though no one has asked for it, my take on this is that blanket use of strncpy doesn't improve security and just reduces performance with some of the ways it is used in DynamoRIO. The func metadata symbol change here would have an OOB or silent truncation, or it always writes 2048 bytes when I would bet that most symbol names are not close to 2047 bytes.

Since this 'copy a string and trust that statements afterwards ensure it is safe for use' is spread around the code base, my opinion is that it would improve security and efficiency to have a string datatype in DynamoRIO with one designed copy operation that all these spots use. One spot to audit, one spot to enforce your coding standards (strncpy or not), hopefully less chance of string bugs.

[derekbruening]

Even though no one has asked for it, my take on this is that blanket use of strncpy doesn't improve security and just reduces performance with some of the ways it is used in DynamoRIO. The func metadata symbol change here would have an OOB or silent truncation, or it always writes 2048 bytes when I would bet that most symbol names are not close to 2047 bytes.

Since this 'copy a string and trust that statements afterwards ensure it is safe for use' is spread around the code base, my opinion is that it would improve security and efficiency to have a string datatype in DynamoRIO with one designed copy operation that all these spots use. One spot to audit, one spot to enforce your coding standards (strncpy or not), hopefully less chance of string bugs.

Most string operations in DR involve constructing a multi-part string via snprintf, not just a copy, though, IIRC.

To your point, it does seem likely that none of the uses of strncpy need or want the nulling of the entire buffer: probably they could all be replaced with snprintf with %s which might help unify all the uses to be snprintf with BUFFER_SIZE_ELEMENTS and NULL_TERMINATE_BUFFER and if truncation is cared about the return value is checked as in the example here.

[ashfordium]

To your point, it does seem likely that none of the uses of strncpy need or want the nulling of the entire buffer

Grepping for strncpy, there seems to be a good amount of strncpy usage. I looked through a few, and I see some that look like they only overfill by one byte at most for the null terminator, but I did see at least one other similar to this where the full buffer is zero filled. Happy to change these also, this PR or another.

[edeiana]

To your point, it does seem likely that none of the uses of strncpy need or want the nulling of the entire buffer

Grepping for strncpy, there seems to be a good amount of strncpy usage. I looked through a few, and I see some that look like they only overfill by one byte at most for the null terminator, but I did see at least one other similar to this where the full buffer is zero filled. Happy to change these also, this PR or another.

Up to you if you'd like to add more changes to this PR or to do it in another.
If more PRs are coming use "Issue: #7274" at the end of your PR description, instead of "Fixes: #7274" (otherwise #7274 will be automatically closed once this PR is merged to master).

Am I understanding correctly that the stringop-truncation errors disappeared after fixing your build dir? If that's the case, don't forget to update your PR description, which still mentions them.

[ashfordium]

Addressed above review comments

[derekbruening]

Thank you for working through all the iterations on this PR and contributing to the project.

[derekbruening]

To your point, it does seem likely that none of the uses of strncpy need or want the nulling of the entire buffer

Grepping for strncpy, there seems to be a good amount of strncpy usage. I looked through a few, and I see some that look like they only overfill by one byte at most for the null terminator, but I did see at least one other similar to this where the full buffer is zero filled. Happy to change these also, this PR or another.

Up to you if you'd like to add more changes to this PR or to do it in another. If more PRs are coming use "Issue: #7274" at the end of your PR description, instead of "Fixes: #7274" (otherwise #7274 will be automatically closed once this PR is merged to master).

Am I understanding correctly that the stringop-truncation errors disappeared after fixing your build dir? If that's the case, don't forget to update your PR description, which still mentions them.

We're happy to take further improvements! Thank you for volunteering. Echoing @edeiana: I could see it either way: all instances of avoiding strncpy excessive writes in one PR seems reasonable.

[edeiana]

Thank you for your contribution!