strcpy causing segfault
 

All, can anyone please explain to me why the following code is seg faulting on the strcpy?

buf[0] = '\0';
s_element->data_value = (totalused / total)*100;
sprintf(buf, "CPU 0\nLoad: %3.2f%%", s_element->data_value);
if ((s_element->t_element.str = malloc(strlen(buf))) != NULL)
strcpy(s_element->t_element.str, buf);



The code is contained in a function that executes approximately every 3 seconds. buf is a member variable of the function with type 'static char[256]'.

s_element is a pointer to a global static struct.

I'm trying to format data (s_element's data_value) into a string to be displayed. I sprintf it into a buffer then allocate space for s_element's char*.

the call to strcpy executes twice and on the third execution (the third time the function is called) it seg faults.

Can anyone lend a hand?,
Thanks,
jpbarto

These are null terminated strings. and strlen doesn't count the null. So

if ((s_element->t_element.str = malloc(strlen(buf) + 1)) != NULL)
strcpy(s_element->t_element.str, buf);

Thanks, but I'm afraid that didn't do it.
I changed the code a bit for debugging purposes:

new code:
s_element->data_value = (totalused / total)*100;
sprintf(buf, "CPU 0 Load: %3.2f%%", s_element->data_value);
fprintf(stderr, "buf is %d\n", strlen(buf));
fprintf(stderr, "buf contains %s\n", buf);
if ((s_element->t_element.str = malloc(strlen(buf)+1)) != NULL){
fprintf(stderr, "str is %d\n", strlen(s_element->t_element.str));
/*strcpy(s_element->t_element.str, buf);
fprintf(stderr, "str contains %s\n", s_element->t_element.str);*/
}

And the resulting output:
buf is 18
buf contains CPU 0 Load: 11.24%
str is 22
buf is 19
buf contains CPU 0 Load: 100.00%
str is 10
buf is 17
buf contains CPU 0 Load: 3.67%
str is 10
buf is 17
buf contains CPU 0 Load: 0.67%
str is 10
buf is 17
buf contains CPU 0 Load: 1.33%
str is 10

Why is malloc creating a string that is 10 when its told to create one that is 17? Or maybe the question should be, where is the null terminator coming from?

sprintf will append the null terminator for you, on buf.

Dont forget str is pointing at assigned memory which could contain anything ( you haven't set it yet ) So the values from

fprintf(stderr, "str is %d\n", strlen(s_element->t_element.str));

are meaningless.

My advice would not be to use this method. set s_element->t_element.str as an array of 25 characters. ( Since this covers the maximum quite confortably. ) and drop the malloc all together. Also are you freeing this memory?

I'm not freeing the memory (perhaps realloc would be better than malloc?) and I wish that I could set s_element->t_element.str to a static array length but its a member of a struct and that struct is used for multiple things (receiving output from 'who' for example).

Ok, so strlen is useless until s_element->t_element.str is set, I can understand that. However why then is the program crashing on the second strcpy? Is it something to do with calling malloc to assign memory to the same pointer without ever freeing the pointer?

Thanks for all your help thus far by the way,
there are similar lines of code in other functions that have given me no problem. In those instances 'buf' is populated by a gets() where as here it is populated by a sprintf... its odd.

leonscape, I have to apologize. It seems I may have been chasing the wrong code. Although running the app doesn't print out a specific line of code as the culprit (it in fact returns an error for chunk_alloc in libc.so.6). I inserted multiple printf statements to trace the app as it flows and it segfaults on an XSync call... odd...

Any recomendations on how I could find out more about the error?

Thanks again for all your help with this,
jpbarto

Alright, some more studying shows that it segfauls on the XSync function (which occurs in another one of my defined functions) but XSync only segfaults if I perform the strcpy.

And buf (of the original posted code) is not the culprit as a strcpy of
strcpy(s_element->t_element.str, "cpu 0\nload: 9.3%");

causes a segfault as well. Is there some attribute of how strcpy functions that I'm forgetting?

What is the call to xsync, are you clearing the queue?

Is the display your calling it on, anything to do with s_element.

Malloc?
 

Ok! The biggest problem that I always run into when using malloc/realloc/free is that I forget to #include <malloc.h> check this first

also you are not adding memory space for the trailing '\0' (+1)

if you are indeed including malloc.h then change your code as follows:

s_element->t_element.str = realloc(s_element->t_element.str, (strlen(buf)+1)*sizeof(unsigned char));
if (s_element->t_element.str) strcpy(s_element->t_element.str, buf);

Someone please correct my if I am wrong, but won't

if ( (s_element->t_element.str = malloc(strlen(buf)) ) != NULL)

always be true? Aren't we saying here "if (assignment) == true) then --"
and the assignment will always be true... will it not? It is safer (I think) to make the if statement seperate from the assignment statement.

Don't use malloc in an operation like that. Use mypt=realloc(mypt, mylen).

mylen + 1 if it is a null terminated string

MAKE SURE THAT s_element->t_element.str IS SET TO NULL BEFORE REALLOC IS CALLED FOR THE FIRST TIME... APPON STARTUP

Hope that helps

Everyone, thanks for the help.

XSync is being called (this is an XScreensaver hack) before the next iteration of the program... I don't know, it was part of the base 'skeleton' hack that I began modding for my own purposes.

Also, Nerd, I tried your suggestion and unfortunately the app still crashed. Althought as I've been playing with this I've noticed some odd but yet consistent behavior.

The following code fails with a seg fault:
sprintf(buf, "CPU 0\nLoad: %3.2f%%", s_element->data_value);
s_element->t_element.str = realloc(s_element->t_element.str,
(strlen(buf) + 1)*sizeof(unsigned char));
if (s_element->t_element.str)
strcpy (s_element->t_element.str, buf);

But this code works just fine:
sprintf(buf, "CPU 0\n %3.2f%%", s_element->data_value);
s_element->t_element.str = realloc(s_element->t_element.str,
(strlen(buf) + 1)*sizeof(unsigned char));
if (s_element->t_element.str)
strcpy (s_element->t_element.str, buf);

If you notice the only difference is the length of the string stored in 'buf'. I even hardcoded malloc (and realloc) to allocate 24 bytes and got the same behavior, so t_element.str allocated with malloc(24) would fail if 'Load' was copied into str but if I reduced the lenght of the string it works ok... any explanations?

jpbarto

The original construct is not wrong, Its used quite a lot, what your saying is

if (( the value assigned) == true )

Thats what the extra brackets are for. Without them then yes it would always return true, with the brackets it means something slightly diffrent.

Now I have though of something, I hadn't though before ( it has been a long while since I used c ) theirs no cast from void*

s_element->t_element.str = (char *)realloc(s_element->t_element.str, (strlen(buf) + 1));

malloc and realloc, return a void pointer, since I think str is a char pointer you need a cast.
this is probably causing the crash. As you maybe corrupting the stack.

leonscape, thanks for the continued advice; but still no dice. I tell you guys what, my next post is going to layout the entire contents of the structs used, and the function using them, perhaps there is something peripheral causing this.

SysInfo.c
 

typedef struct {
/* string to display */
char *str;
/* string length, of course */
int slen;
/* position (in percentage) to place string */
/* where top-left-most of window is 0.0,0.0 and */
/* bottom-right-most of window is 1.0,1.0 */
float pos_x;
float pos_y;
} txt_element;

typedef struct {
Pixmap *img;
float pos_x;
float pos_y;
} img_element;

typedef struct {
txt_element t_element;
img_element i_element;
/* command to populate element */
char *cmd;
/* if no command, then a custom function to populate the element */
void (*func) (void *);
/* double var to store a reading (to be displayed by a guage) */
double data_value;
/* will the string change over the course of the apps lifetime */
BOOL constant;
} scr_element;


static scr_element cpuload = {{NULL,0,0.05,0.4},{NULL,0.05,0.5},NULL,&populate_cpu_load,0,FALSE};


static void
populate_cpu_load (scr_element *s_element)
{
int fd, len;
static char buf[BUFSIZE];
double diffuser, diffnice, diffsys, diffidle, total, totalused;

/* copy over the old *fresh* values to prep for reading new ones */
memcpy(&cpuload_last, &cpuload_fresh, sizeof(cpuload_last));

/* read in the /proc/stat cpu values */
if ((fd = open("/proc/stat", 00))){
len = read(fd, buf, sizeof(buf)-1);
close(fd);

buf[len] = '\0';
len = sscanf(buf, "cpu %ld %ld %ld %ld\n", &cpuload_fresh[0],
&cpuload_fresh[1],
&cpuload_fresh[2],
&cpuload_fresh[3]);
}else
fprintf(stderr, "%s: Failed to open /proc/stat for reading\n", progname);

diffuser = cpuload_fresh[0] - cpuload_last[0];
diffnice = cpuload_fresh[1] - cpuload_last[1];
diffsys = cpuload_fresh[2] - cpuload_last[2];
diffidle = cpuload_fresh[3] - cpuload_last[3];
total = diffuser + diffnice + diffsys + diffidle;
totalused = diffuser + diffnice + diffsys;

s_element->data_value = (totalused / total)*100;
sprintf(buf, "CPU 0\n%3.2f%%", s_element->data_value);
s_element->t_element.str = realloc(s_element->t_element.str,
(strlen(buf) + 1)*sizeof(unsigned char));
if (s_element->t_element.str)
strcpy (s_element->t_element.str, buf);

if (s_element->data_value > 83)
s_element->i_element.img = cguages[6];
if (s_element->data_value > 67)
s_element->i_element.img = cguages[5];
if (s_element->data_value > 50)
s_element->i_element.img = cguages[4];
if (s_element->data_value > 33)
s_element->i_element.img = cguages[3];
if (s_element->data_value > 16)
s_element->i_element.img = cguages[2];
if (s_element->data_value > 0)
s_element->i_element.img = cguages[1];
else
s_element->i_element.img = cguages[0];
}


then there is a 'main' function which calls the 'populate' function approximately every three seconds in order to maintain present CPU load.

ideas?

Thanks for the continuing help on this, I really appreciate it.
jpbarto

Oh, P.S., cguages is just an array of pointers to Pixmaps.

Whats BUFSIZE?

oh, my bad! It is
#define BUFSIZE 2600