1


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

Improve this question
3
  • 1
    I suspect that you have a basic misunderstanding about the difference between a pointer and the thing the pointer points at. When you "assign a string" in C you're only assigning the pointer. The actual character data doesn't move. Commented Dec 9, 2013 at 1:10
  • 1
    Why you immediately free after malloc? You can't access to p1->b after you free buffer too! p1->b is just a pointer point to a buffer, you can only free the buffer when you are sure that will no longer being used. Commented Dec 9, 2013 at 1:14
  • Thanks a lot @moeCake, I'm not freeing buff from within get_data() function anymore, instead in main() at the end im freeing (my_s1.b) and not getting memory leaks or valgrind errors. Commented Dec 9, 2013 at 1:29

2 Answers 2

Reset to default
2

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).

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

0

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
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.