custom strcat() question, is this code safe?

[muratagenc]

I got the following code from a book, it works but I don't think it's a safe function. Why? let me put the code first;

void myStrCat (char * s1, char * s2, int len){ 
	//go to the end of s1
	while (*s1) s1++;

	if(len == -1) len = strlen(s2); //fully concatenation

	while ( *s2 && len){ //if len positive (bec it will be decreased by 1)
		*s1 = *s2; //copy chars

		s1++; //move one forward
		s2++; //move one forward

		len--;
	}
      
       *s1 = '\0'; //null terminate (forgot to put this line..

}

here on line 3, we're iterating to the end of s1.
end on line 8, start copying s2, starting at s1+1.

I think, since we don't know what we have at the location s1+1 in the memory, this code is not safe.

what do you think?

[Catfish]

What do you mean when you say "safe"? Crash safe, or secure, or what.
I think myStrCat(str, "", 30); may crash it.
I think myStrCat(0x002422, 0x000011, -30); may also crash it.
As for security, I once read that strlen() is unsafe.

The moral of the story: the whole assembly needs to be safe, not just one function. You need input checking and validation way before you call your myStrCat().

[muratagenc]

I meant "memory safe". I don't know the terminology, let me explain what I mean;
s1 and s2 are char arrays.

let's say, s1 sits between 10 and 20, s1 length is 8 (terminated with null, the array is not full)
and s2 sits btw 60 and 75. s2 length is 12.

So, when this function adds s2 to s1; total length of s1 should be 8+12 = 20.
but in it's array, it has only 10 units in total. 10 more will be taken from memory (20-30)

since we don't know what we have at 20-30, we're just overwriting to that location, which is what I mean saying "unsafe".
if there is something else at 20-30, some important stuff, it will be destroyed.

right?

and yes, your examples are good, they will crash it. by the way, are we supposed to write our functions crash safe? I don't know about business requirements in general... actually, this would be another topic.

[muratagenc]

I made a search and found that this problem is called buffer overflow. strcat itself has this problem, and "safer" versions take the buffer size as an argument. thanks for the comment Catfish.

[muratagenc]

added buffer size check;

#include <iostream>
#include <cstring>
using namespace std;

void myStrCat( char * s1, char const * s2, int bSize, int len = -1); 

int main(){

	char str1[25] = "This is a test";
	char str2[80] = "hey there!";

	myStrCat(str1, str2, 25, 5); 
	cout << str1 << endl;

	strcpy(str1, "This is a test"); //resetting str1
	myStrCat(str1, str2, 25); 
	cout << str1 << endl;

return 0;
}

void myStrCat (char * s1, char const * s2, int bSize, int len){ 

	if(len == -1) len = strlen(s2);

	if( strlen(s1) + len > bSize){
		cout << "cannot concatenate! quitting...\n";
		return;
	} 

	while (*s1)	s1++;

	while ( *s2 && len){
		*s1 = *s2;
		
		s1++; 
		s2++; 
		len--;	
	}

	*s1 = '\0';
}

[Catfish]

are we supposed to write our functions crash safe?

I call a function "crash safe" when it treats most or all possible errors and will terminate cleanly instead of doing stupid things.

For example MyStrCat() doesn't check if len is a negative value other than -1. (Ignore the random addresses in that example, they're just there to showcase that it's impossible to prevent a crash if someone really wants it to happen.)

What will happen for len == -30? It will keep decreasing len until it underflows and becomes 0. In theory that is. In practice it will try to access forbidden memory and will be stopped long before that.

You can improve your function by using if (len <= -1) instead.
You can improve it even further by using unsigned int len forcing len to be a positive value. And in case it's 0, use the strlen(s2).

Also strlen() returns a value of type size_t which could be unsigned long int depending on the computer and its compiler. This is a minor auxiliary point but you should respect the types and prevent implicit casts.

[muratagenc]

great tips, thanks. I also like the comment This is a minor auxiliary point but you should respect the types and prevent implicit casts. very much. thanks again.