C - create a string "from" struct parameter

Question

Have a

typedef struct person {
    char name[20]
    char surname[20]
} person_t;

I need to create a string like XXXXXX:YYYYYY with the function like char* personToString(person_t *p). I tried to make it:

char* personToString(person_t* p) {

  int n1,n2;
  n1=strlen(p->name);
  n2=strlen(p->surname);
  char *p = (char*) malloc((n1+n2+2)*sizeof(char));
  strcat(p,puser->name);
  strcat(p,":");
  strcat(p,puser->surname);

  return p;
}

This give me a reasonable output but I have some errors testing with valgrind! I also think that there is a way more classy to write the function!

you can use `sprintf` instead of `strcat`, like `sprintf(p, "%s:%s", p->name, p->surname);`, plus u override `p`, u get it as parameter and then declare it as `char *` and you dont `free` your `char *` — Opsenas, Jul 18 '13 at 08:19
Shouldn't it be `struct person *p` instead of `person_t`, since `person_t` is an object and not `typedef`? — mohit, Jul 18 '13 at 08:24
i'm addinf also char ':' so i think i need +2! I tried with +1 and give me errors — user2590319, Jul 18 '13 at 08:27
@RedX +2 is fine he is including ":" also in the final string — Sanyam Goel, Jul 18 '13 at 08:30

score 3 · Answer 1 · answered Jul 18 '13 at 08:19

When you malloc memory for p the memory will hold garbage values. Strcat will append a string after the null character, but in an uninitialized string will hold random values.

Replace the first strcat with strcpy.

score 3 · Answer 2 · answered Jul 18 '13 at 08:20

3

You need to

strcpy(p,puser->name);

not

strcat(p,puser->name);

malloc does not initialize the buffer to zero, so strcat is searching for a null byte in p first and probably not finding one, reading past the end of the buffer and thus crashing.

Instead of one strcpy plus two strcat you can also write one call to sprintf:

sprintf(p, "%s:%s", puser->name, puser->surname);

answered Jul 18 '13 at 08:20

Werner Henze

16,404
12
44
69

you're right :D thank you so much! (st**id error for me) Even if it works well, do you think there's a way more classy to write the code? – user2590319 Jul 18 '13 at 08:25
Werner, You beaten us, your method is more appropriate to concatenate more then one strings. + to your answer. Nice – Grijesh Chauhan Jul 18 '13 at 08:31
@user2590319 I'ld use sprintf. VoidPointer's implementation looks quite good to me. – Werner Henze Jul 18 '13 at 08:31

Grijesh Chauhan · Answer 3 · 2013-07-18T08:25:15.170

2

First you should call string copy, then strcat:

strcat(p,puser->name);

should be:

strcpy(p,puser->name);

because memory allocated with malloc function keeps values garbage, by doing strcat for first you are concatenating after garbage -- it also brings Undefined behaviour in your code.

You can use void* calloc (size_t num, size_t size); instead of malloc(), calloc function initialized allocated memory with 0 (then strcat() no problem). Also dynamically allocated memory you should deallocate memory block using void free (void* ptr);) explicitly.

edited Jul 18 '13 at 08:25

answered Jul 18 '13 at 08:19

Grijesh Chauhan

57,103
20
141
208

Just to add to the answer, Valgrind might be complaining about not `free`ing the memory – Suvarna Pattayil Jul 18 '13 at 08:22

VoidPointer · Answer 4 · 2013-07-18T08:38:01.483

2

This looks good to me,

char* personToString( struct person_t *p )
{
  int len = strlen(p->name) + strlen(p->surname) + 2; // holds ':' + NULL 
  char *str = malloc( len ); // Never cast malloc's return value in C

  // Check str for NULL
  if( str == NULL )
  {
      // we are out of memory
      // handle errors
      return NULL;
  }

  snprintf( str, len, "%s:%s", p->name, p->surname);

  return str;
}

NOTE:

Never cast malloc's return value in C.
Use snprintf when multiple strcat is needed, its elegant.
free the return value str here in caller.

Fixed struct and char variables.

edited Jul 18 '13 at 08:38

answered Jul 18 '13 at 08:26

VoidPointer

3,037
21
25

Shouldn't it be `struct person *p` instead of `person_t`, since `person_t` is an object and not `typedef`? – mohit Jul 18 '13 at 08:28
Nice answer actually I have suggestion change comment *`Never cast malloc's return value`* to *`Never cast malloc's return value in C`* – Grijesh Chauhan Jul 18 '13 at 08:34
No need to snprintf, sprintf would suffice since you made sure the buffer is large enough. BTW, it's p, not puser. And there are too many closing braces in the snprintf line. – Werner Henze Jul 18 '13 at 08:34
Checking p for NULL, than i might set value of errno? Like: `if(p==NULL) { errno = EINVAL; return NULL; }` is it good? – user2590319 Jul 18 '13 at 08:35
@WernerHenze `snprintf()` not needed here (because allocated correct) but it is preferable and safe. – Grijesh Chauhan Jul 18 '13 at 08:36
@user2590319 its updated to handle `malloc` error. Regarding `errno`, refer http://stackoverflow.com/questions/11699596/how-to-set-errno-value – VoidPointer Jul 18 '13 at 08:41

C - create a string "from" struct parameter

4 Answers4