strncpy To strcpy Equivilence in Code

Question

I have this ugly function, and I feel that the entire strncpy should just be an strcpy:

void PackData(char*& cursor, const std::string& data) {
    *(reinterpret_cast<int*>(cursor)) = static_cast<short>(data.length() + 1);
    cursor += sizeof(int);
    // copy the text to the buffer
    ::strncpy(cursor, data.c_str(), data.size());
    cursor += (data.length() * sizeof(char));
    *(reinterpret_cast<char*>(cursor)) = 0;
    cursor += sizeof(char);
}

cursor is guaranteed to have enough room to contain all the data copied. And data only contains a '\0' character at termination.

I want to update this function to use strcpy, and to remove some of the ugly. Here's what I have:

void PackData(char*& cursor, const std::string& data) {
    const int size = data.size() + 1;

    std::copy_n(cursor, sizeof(int), reinterpret_cast<char*>(&size));
    cursor += sizeof(int);
    strcpy(cursor, data.c_str());
    cursor += size;
}

My code works fine, but I wanted to ask if anyone sees any misbehavior that I may have missed?

Well, for purposes like this, I, somehow, feel that `memcpy` would be better. — Algirdas Preidžius, Jan 21 '16 at 12:49
I have to agree that that is ugly. It makes me wonder if the person writing it was somehow overly sensitized to the potential of an out of bounds write. Not that it isn't super bad, but that is a heck of a lot of casting and dancing for a `strncpy`. — David Hoelzer, Jan 21 '16 at 12:54
`memcpy` would be faster, since it doesn't check for `'\0'` each character, but also probably a little less clear since I'm copying a string. I could go either way, but the question would remain, will the output be consistent. — Jonathan Mee, Jan 21 '16 at 12:55
@DavidHoelzer Even the idea of casting an `int` into the start of the string, when you could have just done `strlen` on the other side is nauseating. But I don't control the module this junk is being passed into, so I just want to make sure that I haven't overlooked something, before I check in a change like this. — Jonathan Mee, Jan 21 '16 at 12:58
@skyking I've made a copy of this here: http://codereview.stackexchange.com/q/117494/95369 I didn't even know that the site existed! If I can get some good responses over there I'll have to delete this question. Thanks for the tip! — Jonathan Mee, Jan 21 '16 at 14:24

score 2 · Accepted Answer · edited Jan 21 '16 at 15:38

2

Whoever wrote that code had no idea what they were doing. That use of strncpy makes no sense, since the length passed to it in the call is the length of the source, not the destination. And that reinterpret_cast at the end just casts cursor to its original type. Get rid of this nonsense. Your code is a good replacement.

edited Jan 21 '16 at 15:38

Slava

43,454
1
47
90

answered Jan 21 '16 at 14:48

Pete Becker

74,985
8
76
165

I take your answer to mean that my code should have equivalent behavior. – Jonathan Mee Jan 21 '16 at 14:54
@JonathanMee - yes, do it. I've edited my answer accordingly. Sorry about that bit of carelessness. – Pete Becker Jan 21 '16 at 14:56
It is not equivalent and not a good replacement, as strcpy() copies one character more – Slava Jan 21 '16 at 14:58
@Slava That was the intention as the original function inserted that `'\0'` after the `strncpy` mine does not insert the terminating null. – Jonathan Mee Jan 21 '16 at 15:00
@JonathanMee yes you are right, did not look accurately, missed part where 0 terminator added manually. – Slava Jan 21 '16 at 15:05
@Slava If you are responsible for the downvote on this answer, I'd appreciate you removing it in light of `strcpy` being apparently equivalent to `strncpy`. Alternatively please provide explaination of how the answer falls short. – Jonathan Mee Jan 21 '16 at 15:26
1

@JonathanMee I cannot unless answer was changed – Slava Jan 21 '16 at 15:30

strncpy To strcpy Equivilence in Code

1 Answers1