3

Here is my code

#include <stdio.h>
#include <string.h>
#include <stdlib.h>


int main() {

 char f[] = "First";
 char s[] = "Second";
 char *tmp = malloc(strlen(f) + strlen(s) + 2);
 strcpy(tmp, f);
 strcpy(tmp, s);
 printf("%s", tmp);
 free(tmp);
 return 0;
}

I'm trying to concatenate f and s. The problem is that tmp contains only "Second" as a array. What I miss here

pr1m3x
  • 1,947
  • 6
  • 21
  • 19

7 Answers7

6

strcpy copies the string to the beginning of the destination, you want strcat instead.

Daniel Fischer
  • 181,706
  • 17
  • 308
  • 431
3

The second strcpy overwrites the previous one. Both copy its content to the tmp pointer (at the start of it). You should use tmp+strlen(f).

Or even better use strcat.

And even better use more secure methods like: strncpy, strncat, etc..

duedl0r
  • 9,289
  • 3
  • 30
  • 45
2

If you insist on using strcpy, your code should be slightly modified:

int main() {
    const char *f = "First";
    const char *s = "Second";
    char *tmp = malloc(strlen(f) + strlen(s) + 1);
    strcpy(tmp, f);
    strcpy(tmp+strlen(f), s);
    printf("%s", tmp);
    free(tmp);
    return 0;
}

You should consider using strncpy instead of strcpy for safety reasons. Also, strcat is a more conventional function for concatenating C string.

EDIT Here is an example of using strncpy instead of strcpy

#define MAX 1024

int main() {
    const char *f = "First";
    const char *s = "Second";
    size_t len_f = min(strlen(f), MAX);
    size_t len_s = min(strlen(s), MAX);
    size_t len_total = len_f + len_s;
    char *tmp = malloc(len_total + 1);
    strncpy(tmp, f, len_f);
    strncpy(tmp+len_f, s, len_s);
    tmp[len_total] = '\0';
    printf("%s", tmp);
    free(tmp);
    return 0;
}
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • 2
    There's absolutely no benefits from using `strncpy()` instead of `strcpy()` in this very case. That's like Microsoft forcing functions with `_s` suffix everywhere. – sharptooth Jan 11 '12 at 16:04
  • 3
    `strncpy` should **never** be used unless you're working with fixed-width, not-necessarily-terminated string fields in structures/binary files. – R.. GitHub STOP HELPING ICE Jan 11 '12 at 16:22
  • @sharptooth There is indeed no reason to use `strncpy` in toy cases, such as this one. However, it is very important to get into a habit of using `strncpy` without thinking twice, to avoid using `strcpy` when it may *really* matter. For example, if you change this very program to take `f` and `s` as parameters, switching to `strncpy` becomes imperative in multithreaded environments. Otherwise, malicious code could move the terminating zero in one of the strings between you calling `malloc(strlen(f) + strlen(s) + 1)` and the calls to `strcpy`. You'd need to limit `strlen(f) + strlen(s)` too. – Sergey Kalinichenko Jan 11 '12 at 16:24
  • @R.. I **strongly** disagree with you on this. Please see my reply to sharptooth to see why. – Sergey Kalinichenko Jan 11 '12 at 16:26
  • 1
    Well you're wrong. `strncpy` is not for bounded copying. You're confusing it with the BSD function `strlcpy`. The only easy-to-use, safe, general-purpose string assembly function in C is `snprintf` and unless you really know what you're doing and have a good reason not to, you should always just use `snprintf`. – R.. GitHub STOP HELPING ICE Jan 11 '12 at 16:29
  • @R.. To eliminate guesswork, I added an example of using `strncpy` to implement *bounded copying* in the answer. – Sergey Kalinichenko Jan 11 '12 at 16:45
  • 1
    @R.. I genuinely think that I am missing your point here: could you please explain in what way `memcpy` is better than (or for that matter, is different from) `strncpy` in my example above? As far as I know, `strncpy` would not read past `'\0'` of the source string, should one be inserted after taking the length but before copying, and pad the rest of the output buffer with `'\0'`, while `memcpy` would blindly copy `len` bytes from one place to the other. – Sergey Kalinichenko Jan 11 '12 at 18:55
  • `strncpy` will work here, but using `strncpy` like this is like using `wcschr` to search for an integer in an array of integers. It happens to work, but its use is non-idiomatic and just plain wrong. The behavior of `strncpy` is to copy bytes until either a null byte is encountered or the length limit is reached, then zero-pad the destination to the specified length if the null byte was reached before the length limit was reached. This is complicated behavior that can lead to failure-to-terminate issues or wasteful zero-filling (sometimes making O(1) operations become O(n))... – R.. GitHub STOP HELPING ICE Jan 12 '12 at 01:16
  • `memcpy`, on the other hand, is the standard and idiomatic function to use when you want to copy a specific, known number of chars. It makes it clear to the reader that you know what you're doing and know exactly how many bytes will be read and written. – R.. GitHub STOP HELPING ICE Jan 12 '12 at 01:18
  • 1
    @R.. I understand your desire to bend definitions slightly to suite your argument, but `memcpy` copies a specific known number of *bytes*, not *chars*. This is quite evident from the signature of `memcpy`, featuring prominently a `void*` in the corresponding position. On the other hand, `strncpy` takes *chars* - again, quite conspicuously. But it is OK, I see your point. Thanks! – Sergey Kalinichenko Jan 12 '12 at 02:10
  • Bytes and chars are the same thing, per the C standard. (Actually there are some subtle differences in where/how the terms are used, but they're fully equivalent.) – R.. GitHub STOP HELPING ICE Jan 12 '12 at 03:40
1

You may want to use strcat instead of your second strcpy call, like this:

strcpy(tmp, f);
strcat(tmp, s);

Note also that allocating strlen(f) + strlen(s) + 1 bytes for tmp is sufficient no need to allocate strlen(f) + strlen(s) + 2 bytes. After concatenation, you'll get only one string, so only one null character is required.

ouah
  • 142,963
  • 15
  • 272
  • 331
1

The problem is that you copy the second string in place of the first one (the first parameter of strcpy() is where to copy the string) and this effectively overwrites the first string. Here's an idea of what you need:

size_t firstLen = strlen( f );
size_t secondLen = strlen( s );    
char *tmp = malloc(firstLen + secondLen + 1);
strcpy(tmp, f);
strcpy(tmp + firstLen, s);

This can be achieved by using strcat(), although that would lead to an extra scan along the copied string.

sharptooth
  • 167,383
  • 100
  • 513
  • 979
1

Here is the correct idiomatic safe way to do what you want:

size_t l = strlen(f);
char *tmp = malloc(l + strlen(s) + 1);
strcpy(tmp, f);
strcpy(tmp+l, s);

or:

size_t l = strlen(f) + strlen(s) + 1;
char *tmp = malloc(l);
snprintf(tmp, l, "%s%s", f, s);

I tend to prefer the latter unless you're writing embedded systems code where you want to avoid pulling in printf dependency.

Finally, note that you should be testing malloc for failure, and that it's useless and harmful to allocate memory and copy the strings if all you want to do is print them - you could just do the following:

printf("%s%s", f, s);
R.. GitHub STOP HELPING ICE
  • 208,859
  • 35
  • 376
  • 711
1

using strcat() instead, which means append a string accroding to the MSDN doc.strcpy() just means copy a string. If you don't want to use strcat(), you should point out the position by using strncpy() or strcpy_s(). Please refer to the document.

Sun Keqin
  • 43
  • 1
  • 5