Best approach for safeguarding char[] when getting back from function?

Question

I am maintaining a piece of C code where char arrays are frequently populated by passing them into functions and using the result as a string that is written to output. However there is no checking done on the array after it has been processed by the function and I'm wondering what the best approach to take is?

One approach is to set the last element in the array to \0 after it has been returned but I suspect there are probably better ones.

void Unpack(char* inbuf, char* outbuf);

int main(int argc, char* argv[])
{
    char* inData = "abc";
    char outData[4];
    char result[14];

    Unpack(inData, outData);
    outData[3] = '\0';  // Insert this to safeguard array before using as string.

    _snprintf(result, sizeof(result), "blah %0s blah", outData);
    printf(result);

    return 0;
}

void Unpack(char* inbuf, char* outbuf) {
    for(int index=0; index<3; index++) {
        *outbuf++ = *inbuf++;
    } 
}

Answer 1

Protect them from what? If you're trying to prevent things like buffer overruns, you're too late; the function you called had full access to use and abuse, and there's not much you can do after the fact if the choice was "abuse".

Answer 2

If you own both codes the better approach will be to change Unpack signature to pass the output buffer size, so it can be responsible for appending a '\0' character.

If you don't have control over Unpack I'd write a thin layer which will do the work.

unpack_safe(char *in, char* out, size_t len) {
  unpack(in, out);
  out[len-1]='\0';
}

With this you can only protect about a non null terminated string, but if you string can contains null characters this will not work as expected, and passing the intput size & output size will be the correct behavior.

Answer 3

Your solution to this problem achieves a couple of things, but unfortunately there are plenty of other issues that it doesn't address.

If you are worried that the output array might not even have a valid '\0' terminated C string in it, then you've addressed that issue by inserting a '\0' by brute force. Additionally you've chosen the optimal location, the last byte of the array can't be used for anything else if the array is supposed to hold a '\0' terminated string.

Unfortunately, you've done nothing to stop the called function trampling over memory beyond the array you've allocated for it. That's the number one thing I'd be worried about.

Avoiding memory trampling (aka buffer overflow) isn't rocket science, but it does demand discipline and consistency. Usually the basic idea involved is to never simply pass the address of some memory to be populated, but instead to always accompany that address with the length of the memory block available. Of course the called code must respect the limitation thus imposed on it. It looks like you know about the basic idea since you mention snprintf() which is the classic example of this approach.

Answer 4

With raw pointers there is always a chance of buffer overruns. It is always safe to use wrapper classes like std::string, CString etc.

This holds good for the new code you write. For the existing code like this, you can only pray that it does not crash.

Answer 5

If you are worried about the the END-MARKER like 0x55AA or some reserved character. But, if it is the integrity of the data that you are worried, you can try some header with some check-sum or CRC (cyclic Redundancy Check).

Answer 6

Setting the last element to '\0' will terminate the string correctly, but it won't fix the damage that was done when the function wrote outside the buffer. In that case you only hide a bug, and the program might produce wrong results or crash later in a totally unrelated function call or on return.

In my opinion, it would be better to detect if the function wrote outside of it's buffer and crash the program if it did. That makes finding and thus fixing the error easier. To achieve that you could set the last byte of the buffer to '\0' before calling the function and test (with assert()) if that is still the case when the function returns.

And, as Bill Forster already stated, it's always a good idea to also pass the buffer length to the called functions