Replace strcpy() and strncpy() with (new) opal_string_copy() #5786
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Are we going to do this everywhere? Otherwise, why does this particular code snippet deserve such treatment? Not saying it is bad - just asking why here and not everywhere. |
Member
|
Jeff asked if I'd review the release branch PRs for #5779. I pointed out his fix wasn't safe, so this is the follow up in master to make the fixes actually be safe. |
Contributor
|
I get that - my point was that this same code exists all over the code base, not just in this one place. So if we are going to fix it here, then shouldn't it really go everywhere? In which case, why not put it in an opal_strncpy function so we don't copy these gyrations a hundred times? |
Member
|
I assume Jeff's going to fix all the compiler warnings. I'm only asking that he fix them correctly :). I'm not really happy with this patch. I'd much prefer the explicit set to |
Member
Author
|
Per the commit message, FWIW, I was fixing compiler warnings. Other places didn't trip up compiler warnings... If it's not too onerous, I can fix up other places. Can you cite some of them? |
Contributor
|
Here it is for and now for strncpy: I probably missed a few - my copy/paste got tired. |
Contributor
|
This cries out for a simple search/replace of strncpy with opal_strncpy (and friends), and then fix it in one place. We actually do have an opal_strncpy, you know... |
Contributor
|
Ah, but you see there is a difference. @jsquyres saw one warning and decided to fix it. I see hundreds of them - it all depends on your compiler/environment combination. My point is that fixing one is fine, but this is a one-off fix that doesn't really address the problem. Rather than someone replicating this all over the place, one warning-squash at a time, it would be nice to have a more standardized solution. I will do it for pmix as we have the same problem there. Ditto for the asprintf warnings. It actually turns out to be rather trivial. 😄 |
Contributor
|
FWIW: see how I dealt with it in PMIx: openpmix/openpmix#843. Note that I had to do it that way due to the use of strncpy in user-facing macros. |
Member
|
@jsquyres sigh; I somehow read strncat as strncpy(). Getting old sucks. |
bwbarrett
approved these changes
Sep 27, 2018
Member
Author
|
Ok, I see where Ralph is going with this -- I didn't think he was talking about all We certainly could do something like an
A similar approach could be taken for |
Contributor
|
I'll politely disagree with your first suggestion - the behavior matches expectations, which admittedly may be incorrect based on the latest man pages (I honestly don't know if/when it changed, or if everyone always made the incorrect assumption). You can do as you please in OMPI 😄 In PMIx, the max length of the Your third suggestion is a good one - I'll happily take it! |
Member
|
I'm torn with the opal_strncpy(); generally, we haven't wanted to change behavior with our wrappers, only to make the behavior consistent across implementations. The Linux implementation of strncpy (not adding the With opal_asprintf(), the return is at least undefined; we're just defining undefined behavior. I guess I have a gut reaction that I don't like it, but my reasons are lame enough that I'm not going to fight it. |
No code or logic changes Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
Do essentially the same thing as strncpy(3), but a) ensure to always terminate the destination buffer with a \0, and b) do not \0-pad to the right. Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
In many cases, this was a simple string replace. In a few places, it entailed: 1. Updating some comments and removing now-redundant foo[size-1]='\0' statements. 2. Updating passing (size-1) to (size) (because opal_string_copy() wants the entire destination buffer length). This commit actually fixes a bunch of potential (yet quite unlikely) bugs where we could have ended up with non-null-terminated strings. Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
jsquyres
force-pushed
the
pr/string-madness
branch
from
28bea3b to
0379a44
Compare
Member
Author
|
I just pushed up some new commits (you should look at the individual commit messages):
If people like this approach, I can:
|
This was referenced Sep 28, 2018
gpaulsen
approved these changes
Oct 1, 2018
Member
gpaulsen
left a comment
gpaulsen
left a comment
There was a problem hiding this comment.
I've re-reviewed the 4 new commits with all of the changes.
jsquyres
changed the title
Add a hueristic: if the string copy is "too long", fail an assert(). This is based on the premise that Open MPI doesn't do large string copies. So if we see a dest_len that is over a certain threshhold (currently set at 128K), this is likely a programmer error, and on debug builds, we should fail an assert(). In production builds, it will work just fine (assuming that it's not a programmer error). Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
Contributor
|
Hey @jsquyres - I thought you were going to put a |
Member
Author
|
@rhc54 Yeah, that's the plan -- I didn't want to do it yet, because it would prevent testing what I have done so far. 😉 |
gpaulsen
approved these changes
Oct 4, 2018
Member
gpaulsen
left a comment
gpaulsen
left a comment
There was a problem hiding this comment.
looks great to me!
#error on various strcpy functions can come in future PR.
Member
Author
|
@gpaulsen This PR was not complete. It should not have been merged. 😦 Fortunately, it's not harmful to have been merged, but the PR was only something like 1/6th complete (one set of substitutions on OPAL was done -- the rest of OPAL still needs to be done, and nothing in ORTE or OMPI was done yet). |
Member
|
Sorry. Would you like me to revert it? Is there a convention on who should merge? I saw that the CI had passed and that it had two good reviews, without the work in progress label. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EDIT: This PR completely changed after it was created. It was initially just fixing a single compiler warning. It grew into replacing all
strcpy()andstrncpy()instances with calls to a new function:opal_string_copy().Original description
memset() the string, assert() to make sure it's long enough, and also
use strncat() (which is guaranteed to \0-terminate the string).
Signed-off-by: Jeff Squyres jsquyres@cisco.com
Follow on to #5779 (review)