Getting warning "buffer overrun"

[mrpear2020]

Hello, I have two questions:
1)the program below executes OK but getting one warning:
"buffer overrun while writing to 'result': the writable size is (size_s1+size+s2+1)*1 bytes, but 4 bytes might be written"
Any idea how to fix this?

2)When passing parameters to method char * cat() as shown below, what is the correct way to do it? It is not letting me do cat("string1", "string2") for example.

  #include <iostream>

//Goal:To merge two strings using 'new char'.

char* cat(char* string1, char* string2)
{
	//Get lentgh of strings
	int size_s1 = strlen(string1);
	int size_s2 = strlen(string2);
	//Use the total length of two strings here on the 'new char' command
	char* result = new char[size_s1 + size_s2 + 1];
	//counter i will be used to read character by character and build string1
	int i = 0;
	while (string1[i] == '\0') {
		//result[] will hold both string1 and string2 strings
		result[i] = string1[i];
		i++;
	}

	//Now that result[] contains all characters for the first string, copy characters from string2
	//Increment 1 to index 'i' so that the next index will be ready to hold the next string character
	i += 1;
	//Initialize a counter j which will represent the char array for string 2
	int j = 0;
	while (string2[j] == '\0')
	{    
		//Now result[] which contains string1 will be appended with content of string 2, chara
		result[i] = string2[j];
		i++;
		j++;
	}
	//Now that result[] has all characters for both string1 and string2, add the termination character:
	result[i] = '\0';
	return result;

}

int main()
{       //This call is not being accepted...
	std::cout << cat("day", "light");
}

[Duthomhas]

This is your compiler being awesome for you.

What it means is that your input strings might be zero length, but the way your code is written, the minimal size of your result array must still be 4.

To be clear, you ARE correctly computing the size of the resulting array.

But you are NOT correctly copying data it.
(Is there a reason you are not using strcat()?)

[mrpear2020]

I think I understand what you are saying.
Someone suggested I use:
char* cat(const char* string1, const char* string2)

Indeed now I am no longer getting the buffer overrun. Not sure I can understand why the 'const' change alterered it though.

It is a requirement from academic exercise I must write it using 'new char'.

Thank you,

[Duthomhas]

I am not sure how changing things to const made the warning disappear, but it did not fix the problem.

Look at your conditions on lines 14 and 25. Before exiting each loop, you increment your i variable. You also increment it on line 22. By the time you get to line 33 i will have a minimal value of 3, meaning that result[] must have a minimum length of 4.

As a strong hint, get out a piece of graph paper or something and write your input strings on it, including the null-terminator, and also write a box supporting the correctly-sized result. Then trace through your algorithm and you will see where you have made mistakes.

Good luck!

[Handy Andy]

Hello mrpear2020,

In addition to what Duthomhas has pointed out I noticed some other things.

Lines 14 and 25 say while (string1[i] == '\0'). This will only enter the while loops if "string?[0] == '\0'". If you are lucky enough that the string is empty and string?[0] does contain "\0' then you will enter the while loop once and only once.

The idea is to process each element of the array until you find a '\0' marking the end, so the test would be
while (string1[i] != '\0').

Then you have a problem with line 22 moving "i" leaving an element of "result" untouched and most likely containing a garbage value which may or may not print on the screen.

After line 9 I added this for testing:

std::cout << "\n  Size of string1 " << size_s1 << ", " << "Size of string2 " << size_s2 << '\n'; // <--- Used for testing. 

Just to check the sizes before you create the new array.

You have used the "new" to create an array, but I do not see "delete" anywhere to free that memory when you are finished with it. If I understand this correctly this is a memory leak that needs fixed.

Like an if/else new needs a delete to go with it.

Andy

[Handy Andy]

Hello mrpear2020,

Sorry after I posted I went back and noticed question 2.

I am not sure what the technical answer is, but what I noticed before I compiled the program is that in the function call cat("day", "light") "day" and "light" are being considered constant char pointers in VS2017, but at the function you are trying to receive just a char pointer. When I made the function definition
char* cat(const char* string1, const char* string2) // <--- Changed. the error at the function call went away. Not a big deal here as "string1" and "string2" should be used and never changed in the function. This way the function can not change these strings.

I imagine someone will tell me what I have missed here.

Andy

Edit: typo

[Duthomhas]

Some string functions (such as the one in this assignment) are designed to return a new string. The memory leak comes from not freeing the string when you are finished with it:

int main()
{
  char* s = cat( "day", "light" );  // Allocate a NEW string
  std::cout << s << "\n";  // display it
  delete[] s;  // free it
}

[againtry]

Notwithstanding its importance, without const-ness

#include <iostream>
#include <cstring> // <- EDIT: REFER WARNING FROM @MILLER XYL BELOW

char* cat(char* string1, char* string2)
{
    size_t size_s1 = strlen(string1);
    size_t size_s2 = strlen(string2);
    
    char* result = new char[size_s1 + size_s2 + 1];
    
    size_t i = 0;
    while (string1[i] != '\0') // <--
    {
        result[i] = string1[i];
        i++;
    }
    
    size_t j = 0;
    while (string2[j] != '\0') // <--
    {
        result[i + j] = string2[j];
        j++;
    }
    result[i + j] = '\0';
    
    return result;
}

int main()
{
    char day[]{"day"};
    char light[]{"light"};
    
    std::cout << cat(day, light) << '\n';
}

[MILLER XYL]

Just warn you that 'strlen' is not in <iostream> but <cstring> (string.h).

[againtry]

Warning noted @MILLER XYL and for interest sake here is the reason why it works without the #include <cstring> on systems like mine and not on others:

https://stackoverflow.com/questions/19107845/which-c-header-file-declares-strlen

[dhayden]

mrpear2020, once you get this working, consider changing it to use memcpy(). Your code will be smaller and faster.
http://www.cplusplus.com/reference/cstring/memcpy/

[Duthomhas]

@dhayden
This is a standard homework kind of problem, used to reinforce what was taught about handling arrays and array indices.

If we really wanted to do it with all the fast-n-fancy stuff, it would be something like:

#define _CRT_SECURE_NO_WARNINGS
#include <cstring>

char* cat( const char* s1, const char* s2 )
{
  auto n = strlen( s1 );
  return strcpy( strcpy( new char[ n + strlen( s2 ) + 1 ], s1 ) + n, s2 ) - n;
}

#include <iomanip>
#include <iostream>

int main()
{
  auto s = cat( "day", "light" );
  std::cout << std::quoted( s )  << "\n";
  delete [] s;
}

(There is no way to avoid 2n processing of the argument strings: 1st for length, 2nd for copy.)

[againtry]

std::cout << std::quoted( s ) << "\n";
In the interest of shaving another picosecond off while not contributing to any troll-like egoism it should be '\n'.

[Duthomhas]

If that were where shaving picoseconds mattered, I’d also get rid of the std::quoted().

[againtry]

Well, I thought of that but restrained myself because that is an interface issue and not the guts of the claimed fast-n-fancy offering which I would not be surprised is more feathers than cock :).

[Duthomhas]

It still seems to me that this is aiming for the wrong ocean.

[againtry]

Well it would.

[dhayden]

Duthomhas,

This is a standard homework kind of problem, used to reinforce what was taught about handling arrays and array indices.

Agreed.

What I was hinting at is that once you have the length's of the strings, you can copy exactly the right number of bytes. This is probably faster since memcpy can copy many bytes at once:

char* cat( const char* s1, const char* s2 )
{
    auto n1 = strlen(s1);
    auto n2 = strlen(s2);
    char *dest = new char[ n1 + n2 + 1 ];
    memcpy(dest, s1, n1);
    memcpy(dest+n1, s2, n2);
    dest[n1+n2] = 0;
    return dest;
}

[Duthomhas]

Yes, you and I are on the same page. Remember that strcpy() and memcpy() are both optimized to do that. Your code skips a few bookkeeping things mine does, and is probably a tad faster for it.

[againtry]

Same page, same ocean - what metaphorical genius. Which compiler optimised those wonders of literary skill.