views:

327

answers:

7

for certain functions i want to create a copy of the string within the function and then manipulate this - for some strange reason, i cant get strcpy to work (gives me a segmentation fault) - i've also tried passing the arg as a string, this doesnt work either (g++ throws an error saying it expect a char*)

#include <iostream>
#include <cstring>

using namespace std;
void copy_string(char* stri);

int main ()
{
  copy_string("sample string");

  return 0;
}

void copy_string(char* stri) {
  char* stri_copy;

  strcpy(stri_copy, stri);

  cout << "String: " << stri_copy;

}

im not sure i understand why this is happening.

so my two questions are:

  1. why this is occuring - is there an easy fix?
  2. whats the simplest / most efficient way of creating a local copy of a string passed to a function?

thanks!

+7  A: 
 char* stri_copy;

 stri_copy = (char*)malloc(strlen(stri) * sizeof(char) + 1); 
 strcpy(stri_copy, stri);

You aren't allocating space for stri_copy.

dcp
@dcp: Can you show use how to allocate space for stri_copy?
quamrana
@quamrana: stri_copy = (char*)malloc(strlen(stri) * sizeof(char) + 1);That's the "C" way. Or you could use the "new char[ len + 1]" way (already mentioned in another answer) if you want to use the C++ way.
dcp
@dcp: I meant by editing your answer - formatting in comments leaves something to be desired. Once you've done that, you could show us how to delete the memory afterwards as well.
quamrana
@ quamrana: Thats the problem with the C interface. There is no cocept of ownership. You need to define weather the function returns a newly allocated string or uses an internal buffer and then document who is supposed to free the memory. Unfortunately it is never clear in C.
Martin York
the `sizeof(char)` is entirely unnecessary since it is defined by the standard to always be `1`.
Evan Teran
@dcp: The C++ way is to use `std::string`, not `new char[x]`. Really, C-style strings are clumsy and error-prone, and should be used only when clearly necessary.
David Thornley
+1  A: 
char* stri_copy;
strcpy(stri_copy, stri);

The problem is that stri_copy does not point to valid memory. strcpy first parameter expects the proper memory location.

int len = strlen(stri);
char* stri_copy = new char[ len + 1];
strncpy(stri_copy, stri, len );
stri_copy[len] = '\0';
aJ
thanks!ive been doing it through char stri_copy[len]; ...stri_copy [len+1] = '\0', but was hoping there was an easier way :)
v_a_bhatia
@aJ: You still don't need the strncpy and strlen, strcpy does all this for you. You will need to delete the new memory at some time to avoid a memory leak.
quamrana
you need to strlen for the new. Once you have the length it is better to use strncpy (which is always safe), rather than strcpy (which can be unsafe).
doron
@deus: No, `strncpy()` is not always safe. It doesn't necessarily null-terminate the string. There is no good checked string copy in the C string library.
David Thornley
+1  A: 

To use strcpy, you need a buffer of allocated memory as a target. Your stri_copy pointer is not pointing to such a buffer.

Nemanja Trifunovic
+4  A: 

The pointer to stri_copy has not been malloc'd. Using strdup below will solve the problem and allocate the memory accordingly for the value of stri.

char* stri_copy;
stri_copy = strdup(stri);

Hope this helps, Best regards, Tom.

tommieb75
By the way it should be noted that once you are done with the stri_copy variable, you should free it as if you don't you will have a memory leak.
tommieb75
brilliant. thanks very much!
v_a_bhatia
`strdup()` is not a standard C++ function, last I looked. It may or may not be available.
David Thornley
@David: strdup is in fact a C function which resides in string.h.
tommieb75
@v_a_bhatia: You're welcome. Have a peaceful new year to you. :)
tommieb75
@tommieb75: It isn't a standard C90 function. It may be in string.h for a given implementation (variable names beginning with "str" belong to the implementation, I believe), but it isn't in the standard. It's a good addition, I'll admit.
David Thornley
@David: That's interesting. strdup is not ANSI, but in POSIX.. http://www.users.pjwstk.edu.pl/~jms/qnx/help/watcom/clibref/src/strdup.html and http://stackoverflow.com/questions/482375/c-strdup-function and http://www.opengroup.org/onlinepubs/000095399/functions/strdup.html.That's interesting. Thanks for the heads up. Have a happy and peaceful new year! :)
tommieb75
A: 

strcpy does not allocate any storage to hold the result. You're using a "random" pointer as the destination which is why you are seeing segfaults. A very simple implementation of strcpy would look like

void naivestrcpy(char* destination, const char* source) {
   while(*source) *destination++ = *source++;
   *destination = 0;
}

strcpy does exactly what it says on the tin, it copies. It does not ensure you've loaded the right size of paper into the xerox tray.

Logan Capaldo
@Logan: You've told us why we get the segmentation fault, but what does nativestrcpy have to do with it? There are simpler implementations of strcpy.
quamrana
naive, not native. The point is to just show what strcpy does, in that it is very simple. If the OP were to "inline" this definition instead of calling strcpy it may be easier to see why it failed. Looking at a real world implementation might have confused the issue as they tend to use more "tricks".
Logan Capaldo
+1  A: 

You have a segmentation fault because stri_copy does not point to valid memory.

If you can use the STL, then you can have a way to do this:

void copy_string(const std::string& stri) {
  char* stri_copy= stri.c_str();

  // work with the copy of the string

  std::cout << "String: " << stri_copy;

}

The std::string makes a copy of the string parameter and disposes of the copy for you after you've finished with it.

Edit: Used const std::string& as the parameter type.

quamrana
+4  A: 

I don't know if this is what you're looking for but I got it to work with strings.

#include <iostream>
#include <cstring>
#include <string.h>
using namespace std;
void copy_string(string);

int main ()
{  
  copy_string("sample string");

  return 0;
}

void copy_string(string stri) {
  string cpy = stri;
  cout << "String: " << cpy;

}
dpeterman
If it isn't what he's looking for, it should be. Upvoting.
David Thornley
@dpeterman: You seem to be copying the string twice: once as the parameter, and again inside the function.
quamrana
I know, I figured that this may be a piece of the function and that he had to do this. I would just cout<<"sample string" or a variable holding the string, I can try to figure something else out that uses sprintf, but there doesn't seem to be a reason to. If the asker needs to use cstrings then I would like to know what the reason is.
dpeterman
Why use C++ if you're only going to use a portion of it? Use string.h if you need to perform tasks with strings.
dpeterman
I don't have enough rep, but the last edit deserves 1000 downvotes.
Derrick Turk
can you explain? he asked for the easiest way, it works and I only had to change one line of his code...I get blasted for using strings so I show how to without using string and get blasted again
dpeterman
@dpeterman: Because what you wrote is WRONG. It has the same problem as the original: `strcpy()` doesn't allocate memory, so you're copying the string over some random memory (which is likely to segfault). Moreover, if you were allocating memory (to make it legal) you aren't doing anything with it, so you're leaking memory.
David Thornley
so why not use the original one that works with strings...I understand that it was a little inefficient but its what the guy wanted...he said that he tried strings and it didn't work (probably because he didn't change the char* to a string in the declaration) so this fixes that
dpeterman
just a question...would this work?char stri_cpy[255]sprintf(stri_cpy, stri)
dpeterman
nevermind...thats a dumb question
dpeterman