After calling strcpy, the second char array becomes empty

Question

Here is the function prototype in my program:
void FindRepStr(char str[], const char findStr[], const char replaceStr[]);

It find the findStr[] in str[] and replace it with replaceStr[].

Here is my code:

void FindRepStr(char str[], const char findStr[], const char replaceStr[])
{
  char *s = nullptr;
  s = strstr(str,findStr); //s points to the first-time appear in str

  char tmp[] ="";

  //length equal
  if(strlen(findStr)==strlen(replaceStr))
  {
    for(int i=0;i<strlen(findStr);i++)
    {
        if(replaceStr[i]=='\0' || s[i] =='\0')
         break;
        else
            s[i] = replaceStr[i];
    }

    cout<<str<<endl;
  }

  else
  {
  //find shorter than replace
    if(strlen(findStr)<strlen(replaceStr))
    {
        //!!!problem here!!!
        strncpy(tmp,s,strlen(s)+1); // store the left part

        for(int i=0;i<=strlen(replaceStr);i++)
        {
            if(replaceStr[i]=='\0') //if end of replace
            {
                s[i]='\0';    //make s(str) end here
                break;
            }
            else
                s[i] = replaceStr[i];   //if not end, give the value
        }

    }

    //finder longer than replace
    else
    {
        //...not finished yet
    }
  }
}

I haven't finished this but here after strncpy, I printed s and tmp for test and I found tmp is correctly copied, but s print out empty:

cout<<"s before strncpy:"<<s<<endl;
strncpy(tmp,s,strlen(s)+1);
cout<<"tmp after strncpy:"<<tmp<<endl;
cout<<"s after strncpy:"<<s<<endl;

The output:

But in simple test program I write, I found it won't be emptied:

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

int main()
{
  char a[]="abc";
  char b[]="defgh";

  cout<<"a before:"<<a<<endl;
  cout<<"b before:"<<b<<endl;

  strncpy(a,b,strlen(b)+1);

  cout<<"a after:"<<a<<endl;
  cout<<"b after:"<<b<<endl;

  return 0;
}

The output:

What went wrong in my program?

1) Don't tag a question that is clearly C as C++. In C++, you should use `std::string` and not write all this low-level manipulation code. 2) `for(int i=0;i — Sebastian Redl, Mar 12 '18 at 09:17
You could always optimize by calling calling `strlen(findStr)` once before getting into loop, get the length in a temporary variable and use it in loop instead of `for(int i=0;i — WedaPashi, Mar 12 '18 at 09:17
but there are `cout` statements and the compiler is C++ so it's C++ C-style but it's C++: https://meta.stackoverflow.com/questions/360709/how-to-tag-questions-about-c-with-a-little-c — Jean-François Fabre, Mar 12 '18 at 09:18
Please provide a MCVE https://stackoverflow.com/help/mcve . Futhermore, how do you call your function ? Because I don't see any check o see if s is NULL or not. — Tom's, Mar 12 '18 at 09:20
@Jean-FrançoisFabre: OP tagged the question as *both* C and C++. The correct answer for "C" would be to figure out what breaks his `strcpy()`. The correct answer for "C++" would be "use std::string". So it's not a non-issue... — DevSolar, Mar 12 '18 at 09:21
that's why there's a meta question about it. in that case OP is using C++ with libc primitives. If OP used real C++ he wouldn't have so many issues... — Jean-François Fabre, Mar 12 '18 at 09:25
Seconding chqrlie. 1) `strncpy()` *always* "fills up" the string with zeroes, and more importantly, 2) if the source string is too long for the destination, *the destination will **not** be zero-terminated*. — DevSolar, Mar 12 '18 at 09:44

score 5 · Accepted Answer · answered Mar 12 '18 at 09:20

char tmp[] ="";

Here you are creating a character array with enough space to hold the string literal, including the terminating nul. Since the string literal is empty, this character array holds exactly one character.

If you write more than that (and you do), you enter the realm of undefined behavior. Basically you are dumping your strings all over random places in the stack; predictably, this doesn't end well.

You need to ensure your character array has enough space to do what you want.

Also, your program logic looks entirely broken. I don't see how that code is supposed to do what the function name suggests it should.

score 3 · Answer 2 · answered Mar 12 '18 at 09:21

char tmp[] ="";

This declares a local array of one char (it just contains the '\x0' terminator).

Copying more than one character into this array causes undefined behaviour: in this case it's destroying the stack frame storing your local variables.

If you want a pointer, declare char *tmp. Better yet, just use std::string for all of this.

After calling strcpy, the second char array becomes empty

2 Answers2