Memcpy, string and terminator

Question

I have to write a function that fills a char* buffer for an assigned length with the content of a string. If the string is too long, I just have to cut it. The buffer is not allocated by me but by the user of my function. I tried something like this:

int writebuff(char* buffer, int length){
    string text="123456789012345";
    memcpy(buffer, text.c_str(),length);
    //buffer[length]='\0';
    return 1;
}
int main(){
    char* buffer = new char[10];
    writebuff(buffer,10);
    cout << "After: "<<buffer<<endl;
}

my question is about the terminator: should it be there or not? This function is used in a much wider code and sometimes it seems I get problems with strange characters when the string needs to be cut.

Any hints on the correct procedure to follow?

Solution 1

A C-style string must be terminated with a zero character '\0'.

In addition you have another problem with your code - it may try to copy from beyond the end of your source string. This is classic undefined behavior. It may look like it works, until the one time that the string is allocated at the end of a heap memory block and the copy goes off into a protected area of memory and fails spectacularly. You should copy only until the minimum of the length of the buffer or the length of the string.

P.S. For completeness here's a good version of your function. Thanks to Naveen for pointing out the off-by-one error in your terminating null. I've taken the liberty of using your return value to indicate the length of the returned string, or the number of characters required if the length passed in was <= 0.

int writebuff(char* buffer, int length)
{
    string text="123456789012345";
    if (length <= 0)
        return text.size();
    if (text.size() < length)
    {
        memcpy(buffer, text.c_str(), text.size()+1);
        return text.size();
    }
    memcpy(buffer, text.c_str(), length-1);
    buffer[length-1]='\0';
    return length-1;
}

Solution 2

If you want to treat the buffer as a string you should NULL terminate it. For this you need to copy length-1 characters using memcpy and set the length-1 character as \0.

Solution 3

it seems you are using C++ - given that, the simplest approach is (assuming that NUL termination is required by the interface spec)

int writebuff(char* buffer, int length)
{
  string text = "123456789012345";
  std::fill_n(buffer, length, 0); // reset the entire buffer
  // use the built-in copy method from std::string, it will decide what's best.
  text.copy(buffer, length);
  // only over-write the last character if source is greater than length
  if (length < text.size())
    buffer[length-1] = 0;
  return 1; // eh?
}

Solution 4

char * Buffers must be null terminated unless you are explicitly passing out the length with it everywhere and saying so that the buffer is not null terminated.

View more solutions

Comments

[DumbCoder]

If you are using C++ strings use string inseatd of char * and use copy rather than memcpy

[pmg]

If you are trying to write a multi-language source file, you should stay away from "C++isms": no std::, no new, no << (except maybe if you mean bitwise shift), ...

[matth]

writebuff should do whatever it is advertised to do. If the caller expects a terminator, then writebuff must supply it. If the caller expects no terminator then writebuff must not supply it. In this specific case, the caller clearly expects a terminator (operator<<(ostream, char*) expects a terminator).

[Mel]

So this is basically strlcpy(3) or am I missing something?

[matth]

Conversely, if you don't want to treat the buffer as a string, you should not NUL terminate it.

[Mark Ransom]

It's OK to copy length instead of length-1 characters, and half avoids a bug when you pass a buffer length of 0.