Assigning strings to a struct member (pointer) in C

Question

I need to be able to fill out my my_s1 structure with data. I am passing it to a get_data() function that should do all the dirty work. The problem I have is the b member of my structure which is a pointer. I have no idea how to properly assign value pointed by (char *) buff to b without segmentation faults or valgrind errors.
For example:

1. Why does initial p1->b="abc"; works fine, but if i try to strcpy() or assign through "=" operator an array to p1->b i get errors?

2. Does s1 my_s1 allocate memory for b or should I somehow malloc() p1->b myself? But then again i need to free() it and assign a NULL pointer before returning from the function, which defeats the purpose (of having the function assigning data to structure), right?

3. With current code listed below I have "proper execution result" but I also get the following valgrind output errors (from what I understand, please correct me if I'm wrong, it seems as if printf() accesses not properly allocated memory - so it works in this case but it's rubbish) :

valgrind:

==1067== Memcheck, a memory error detector
==1067== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==1067== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==1067== Command: ./if
==1067== Parent PID: 1059
==1067==
==1067== Invalid read of size 1
==1067==    at 0x4E7ADF9: vfprintf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4E83E38: printf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4005EF: main (iface.c:10)
==1067==  Address 0x51f3040 is 0 bytes inside a block of size 5 free'd
==1067==    at 0x4C294C4: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-
linux.so)
==1067==    by 0x40064D: get_data (ifacelib.c:17)
==1067==    by 0x4005D3: main (iface.c:8)
==1067==
==1067== Invalid read of size 1
==1067==    at 0x4EA9459: _IO_file_xsputn@@GLIBC_2.2.5 (in /usr/lib64/libc-2.17.
so)
==1067==    by 0x4E7ADB1: vfprintf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4E83E38: printf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4005EF: main (iface.c:10)
==1067==  Address 0x51f3043 is 3 bytes inside a block of size 5 free'd
==1067==    at 0x4C294C4: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-
linux.so)
==1067==    by 0x40064D: get_data (ifacelib.c:17)
==1067==    by 0x4005D3: main (iface.c:8)
==1067==
==1067== Invalid read of size 1
==1067==    at 0x4EA946C: _IO_file_xsputn@@GLIBC_2.2.5 (in /usr/lib64/libc-2.17.
so)
==1067==    by 0x4E7ADB1: vfprintf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4E83E38: printf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4005EF: main (iface.c:10)
==1067==  Address 0x51f3042 is 2 bytes inside a block of size 5 free'd
==1067==    at 0x4C294C4: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-
linux.so)
==1067==    by 0x40064D: get_data (ifacelib.c:17)
==1067==    by 0x4005D3: main (iface.c:8)
==1067==
==1067== Invalid read of size 4
==1067==    at 0x4EBBDDE: __GI_mempcpy (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4EA939C: _IO_file_xsputn@@GLIBC_2.2.5 (in /usr/lib64/libc-2.17.
so)
==1067==    by 0x4E7ADB1: vfprintf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4E83E38: printf (in /usr/lib64/libc-2.17.so)
==1067==    by 0x4005EF: main (iface.c:10)
==1067==  Address 0x51f3040 is 0 bytes inside a block of size 5 free'd
==1067==    at 0x4C294C4: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-
linux.so)
==1067==    by 0x40064D: get_data (ifacelib.c:17)
==1067==    by 0x4005D3: main (iface.c:8)
==1067==
==1067==
==1067== HEAP SUMMARY:
==1067==     in use at exit: 0 bytes in 0 blocks
==1067==   total heap usage: 1 allocs, 1 frees, 5 bytes allocated
==1067==
==1067== All heap blocks were freed -- no leaks are possible
==1067==
==1067== For counts of detected and suppressed errors, rerun with: -v
==1067== ERROR SUMMARY: 10 errors from 4 contexts (suppressed: 2 from 2)

Code in 3 files.

ifacelib.h:

#ifndef IFACELIB_H
#define IFACELIB_H

typedef struct
{
    int a;
    char * b;
} s1;

int get_data(s1 *);

#endif

ifacelib.c:

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

int get_data(s1 *p1)
{
    char *buff;
    p1->a=1;
    p1->b="abc";

    buff = (char *) malloc(strlen("test")*sizeof(char)+1);
    strcpy(buff, "test");

    p1->b = buff;

    free(buff);
    buff = NULL;

    return 0;
}

iface.c:

#include "ifacelib.h"
#include <stdio.h>

int main()
{
    s1 my_s1;

    if ((get_data(&my_s1))==0)
    {
        printf("a= %d\tb= %s\n", my_s1.a, my_s1.b);
    }

    return 0;
}

Any help or just pointing in a right direction would be appreciated.

From best practices point of view, when working with structures should I be writing a function that fills out data in structure (works on a passed structure) and returns int to control successes/fails or should I be writing a function that returns a modified structure instead ?

This is my first post here, so please bear with me, my formatting mistakes, walls of text and my ignorance.
Thanks in advance,
Tom

Answer 1

You are doing wrong, and just lucky to get correct result, in fact you are accessing memory that just being freed.
It's right that you have to malloc for the char* in struct(by the way, you can use strdup), but you need another destructor to free the struct when their job are done.
In your case, you need a function like free_s1 after the printf, rather than free in constructor function(get_data).

Answer 2

This works in C, not so good in C++:

If you are going to malloc(), it may be better to malloc both your structure and the data area in one blow. Rather than having a pointer arrange a minimal data area at the end of the struct. When allocating the struct add additional bytes to allow for the data. You are then good to go. One free will release both struct and data.

Reworking fragments of your code to show the key idea I get the following. I leave compiling, debugging, even syntax checking, for the student.

typedef struct
{
    int a;
    char * b;
    char   data[1];    // data goes here. 
                       // structure MUST be malloced at run time WITH
                       //  extra storage for data.
} s1;

    s1   *p1;
    data = "test";
    data_len = strlen(data);       // additional bytes of storage
    p1 = malloc( data_len + sizeof( *p1 ) );  // allocate structure + data
    strcpy(s1->data, data );        // copy data to buffer

    ...
    free(p1);        // free storage