-2

in the following code when i use fwrite its giving correct o/p. While memcpy is not working.

typedef struct
{
  char *p1;
  char *p2;
} node;
char s[] = "hello";
char t[] =" there";
node t1, t2;
char str;
FILE op_f;

t1.p1 = malloc(sizeof(strlen(s));
t1.p2 = malloc(sizeof(strlen(s));

t2.p1 = malloc(sizeof(strlen(s));
t2.p2 = malloc(sizeof(strlen(s));

t1.p1 = s;
t1.p2 = t;
copy(&t1,&t2);

str = malloc(sizeof(strlen(s) + strlen(t));
/* gives o/p hello there */
fwrite(t2.p1,1,strlen(s),op_f);
fwrite(t2.p1,2,strlen(s),op_f);

/* gives o/p there */
memcpy(str,t2.p1,strlen(s));
memcpy(str,t2.p2,strlen(s));

Is there any way to copy buffer to str ??

PS: above code is just for reference not the actual code

dascandy
  • 7,184
  • 1
  • 29
  • 50
arccc
  • 17
  • 5
  • 3
    Remember: you need to "malloc(sizeof(strlen(s)+1);" Don't forget the null byte ;) – paulsm4 Jul 20 '11 at 07:00
  • ya got it but still str is overwriting – arccc Jul 20 '11 at 07:02
  • `char str;` should probably be `char *str;` (you want a pointer to `char`s not just a single `char`) – James Jul 20 '11 at 07:02
  • 5
    Essentially, everything in this code is wrong. Every single statement contains errors or relies on erroneous assumptions. – Konrad Rudolph Jul 20 '11 at 07:07
  • You probably shouldn't post 'code for reference' that you haven't actually compiled and run. I doubt that the `fwrite` calls in your code actually produce the output "hello there", because you pass the same pointer `t2.p1` each time. – James Jul 20 '11 at 07:09
  • what did `copy()` do? I see some unpleasant ellipsis and memory leak in your code :p – Stan Jul 20 '11 at 07:10
  • @James: his files writes work because of overlapping bugs (notice the second call has the element size at 2), whats works is, he never opened a file to write to as osgx pointed out. – Necrolis Jul 20 '11 at 07:16
  • @Necrolis: how does passing the same pointer `t2.p1` to `fwrite` first write 'hello', and then write 'there'? It would just write the same content twice, wouldn't it? Anyway, there are so many things in this code which would fail, I was just picking one. – James Jul 22 '11 at 01:24
  • @james: the second call has an element size of two, so it'll write 10 bytes, which means its writing more than just the first string, basically, its just a bug hiding other bugs... I suppose with this there is enough bugs for everyone to have one :p – Necrolis Jul 22 '11 at 05:01

5 Answers5

2

You should declare str as char array, not a single char:

char str[500];

when str is declared as array, the str is a pointer (to the first element of array). And in your code - str was a char, but not a pointer. Memcpy needs pointers as first and second arguments

Also, use malloc for strings without sizeof:

 t1.p2 = malloc(strlen(s)+1);

Also, fwrite is used incorrectly, because the op_f is not initialized with fopen like:

 op_f = fopen("filename.txt", "w");
osgx
  • 90,338
  • 53
  • 357
  • 513
2

Strings stored in char arrays need one extra byte for a terminator. When computing the size you need to add 1 extra position:

t1.p1 = malloc(strlen(s) + 1);

This code

t1.p1 = s;

doesn't copy the text from s to p1, but sets p1 to point to the same string as s.

This part

str = malloc(strlen(s) + strlen(t));

doesn't work because str is a single char and not a char*. You could try

char* str = malloc(strlen(s) + strlen(t) + 1);

If you really intend this to be tagged C++, you should consider using std::string instead. It handles all of these details.

Bo Persson
  • 90,663
  • 31
  • 146
  • 203
1

I just noticed you use 'char str' instead of 'char *str', so the result of malloc (which is a pointer to string) it not correctly stored in the variable.

Also, when you malloc, you need to calculate the size the following way: strlen(s) + strlen(t) + 1. The extra byte is for the terminating NULL character. And you don't need to use sizeof there. The finally statement will look like: 

str = malloc(strlen(s) + strlen(t) + 1);
urish
  • 8,943
  • 8
  • 54
  • 75
1

firstly, you should be using strcpy when working with strings (if they are null terminated), as your code currently has a bug which is hidden by the fact 'hello' and 'there' are the same length, to fix it you should be doing (same applies to the malloc calls):

fwrite(t2.p1,sizeof(char),strlen(t2.p1),op_f);
fwrite(t2.p2,sizeof(char),strlen(t2.p2),op_f); //was also a bug here, you used p1 instead of p2 and the size of each element should have been 1

/* gives o/p there */
memcpy(str,t2.p1,strlen(t2.p1));
memcpy(str,t2.p2,strlen(t2.p2));

Your real problem originates because memcpy doesn't increment pointers, hence you should be doing:

strcpy(str,t2.p1);
strcat(str,t2.p2);

or if you really want to use memcpy:

memcpy(str,t2.p1,strlen(t2.p1));
memcpy(str + strlen(t2.p1) - 1,t2.p2,strlen(t2.p2));

finally, your malloc's return pointers, not a char, so str should be a char*.

Necrolis
  • 25,836
  • 3
  • 63
  • 101
1
/* These are static.  You SHOULD NOT write to either s or t! */
char s[] = "hello";
char t[] =" there";

typedef struct
{
  char *p1;
  char *p2;
} node;
node t1, t2;

/* I think you wanted "*str" here... */
char *str;

/* You definitely want "length of string + 1" here */
t1.p1 = malloc(strlen(s) + 1);
strcpy (t1.p1, s);
...
/* strcpy() and strcat() might be applicable here */
/* strncpy() and strncpy() might be even better - it depends... */
str = malloc(strlen(s)+1 + strlen(t)+1);
strcpy (str, s);
strcat (str, t);
paulsm4
  • 114,292
  • 17
  • 138
  • 190
  • It's fine to write to `s` and `t` if you no longer want them to contain "hello" and "there" - as long as you don't exceed the size of the arrays (six chars each, including the null terminator). – Michael Burr Jul 20 '11 at 07:23
  • No it's not fine - some platforms will store "hello\0" and "there\0" in read-only memory. – paulsm4 Jul 20 '11 at 16:05
  • the literals maybe, but the arrays `s` and `t` are modifiable copies of the literals. – Michael Burr Jul 20 '11 at 16:40