strcpy and strncpy safety stuff

[AbidingDude]

So, the ability to overflow with strcpy() is fairly obvious, and I see how strncpy() isn't the best answer. It won't necessarily null-terminate, and it'll wastefully pad with NULLs. Also, these functions don't offer any kind of error reporting. What about this for a safe string copy? It copies only what's necessary, guarantees null-termination, and when an overflow is detected, returns how many characters past the buffer the source went - albeit a little oddly. It uses the negative portion of the integer range to express how many. The function returns:
positive on success
0 for null pointers for either source or destination
negative for overflow (the absolute value of which is how many bytes over)

#include <string.h>

int strncpy0(char *dst,const char *src,int max)
{
	int i;
	size_t j;

	if(dst==NULL || src==NULL)
		return 0;

	for(i=0;(dst[i]=src[i])!='\0' && i<max; i++)
		;

	if(i>=max){
		dst[max-1]='\0';
		j=strlen(src);
		return i-j-1;
	}

	return i;
}

 

[xwb]

C evaluation is from left to right. You will get an array OOB before i < max. The loop termination condition should be

for(i=0;i < max && (dst[i]=src[i])!='\0'; i++)

 

[SamBones]

It's defined in the C standard that functions return zero for success. This gives you more values to spell out the specific error that occurred. Functions often have one way to succeed, and many ways to fail.

Personally, I would use...
[tt]
0 = Success.
-1 = Failure (NULL ptr).
>0 = Positive value for number of characters not copied.
[/tt]
This lets you use the positive number returned to know how many you still need to copy without having to flip the sign. That makes the return value a little more useful. Just use it.

I also agree with xwb's comment.

Last, you have a couple issues if this gets called with 'max' being 0 (zero). Your original code would still copy one character. xwb's code fixes that, but you would still end up trying to write a NULL byte at [tt]dst[-1][/tt] which can cause all kinds of unintended weirdness. Also, if called with 'max' being a negative number, it will also get messy. Maybe return a success immediately if max==0, and return negative error code if max<0.

 

[xwb]

Sam - strncpy returns the destination string. I think the op is just following that.

 

[AbidingDude]

C evaluation is from left to right. You will get an array OOB before i < max...

D'oh. Thank you for catching that.

It's defined in the C standard that functions return zero for success...

Oh? I thought it was only a requirement for main(). I don't have a copy of the standard. That's just what I heard.

 

[SamBones]

Yeah, it's not a hard and fast requirement as it's pretty easy to find exceptions. The basic idea behind it is that most functions have one way to succeed, and many ways to fail, so 0 for success and non-zero for any failure fits that. The problem I have with it is readability. A successfull zero is a boolean FALSE. You have to negate/not the return value to use it as a boolean value...

if ( [b]![/b] some_function() )
    // It succeeded
else
    // It failed

I do like what you're proposing, but I do like negative for errors and positive for count of leftovers. That gives you use cases like this...

rv = strncpysafe(destination, source, strlen(source));

if(rv < 0) return(FAILURE);

if(rv) {
    printf("Leftovers: %s\n", &source[rv]);
    // Copy more, copy somewhere else, log it, whatever
    // It's nice knowing how much didn't make it
    }

 

[AbidingDude]

The problem I have with it is readability. A successfull zero is a boolean FALSE...

Exactly. It drives me nuts to see:

if(!strcmp(a,b))

Even the C FAQ suggests against it (17.3), so I tend to base my return values on what feels the most intuitive.

 

[AbidingDude]

Ok. Here's the revision:

int strncpy0(char *dst,const char *src,int max)
{
	int i;
	size_t j;

	if(dst==NULL || src==NULL || max<1)
		return -1;

	for(i=0;i<max && (dst[i]=src[i])!='\0'; i++)
		;

	if(i>=max){
		dst[max-1]='\0';
		j=strlen(src);
		return j-i+1;
	}

	return 0;
}

It returns:
-1 on bad input
0 on success
positive for buffer overflow (positive value being the number of bytes over).

 

[AbidingDude]

Now that I think about it, seeing as the whole negative range is freed-up, I might as well take advantage of that and add greater specificity to the return values:

int strncpy0(char *dst,const char *src,int max)
{
	int i;
	size_t j;

	if(dst==NULL)
		return -1;
	if(src==NULL)
		return -2;
	if(max<1)
		return -3;

	for(i=0;i<max && (dst[i]=src[i])!='\0'; i++)
		;

	if(i>=max){
		dst[max-1]='\0';
		j=strlen(src);
		return j-i+1;
	}

	return 0;
}

So now it returns:
-1 on bad dst
-2 on bad src
-3 on bad max
0 on success
>0 on attempted buffer overflow

 

[SamBones]

I like it!

My only remaining heartburn is the name. I like descriptive names since I'm old and forgetful. I would prefer something like [tt]strncpysafe()[/tt]. But it's your function. You can name it as you wish.

 

[AbidingDude]

I'm not in love with the name. I'm open to others.