why does this segfault??

D

deejaybags

could someone please have a look at this and tell me why it segfaults. i
am confused as all hell!

stringman.h
void load_string(char *newString);
char * remove_string();
char * remove_upper_string();
char * remove_lower_string();

(i havent written the removes yet);

then the source
stringman.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* fundamental structure definition */
typedef struct list_member{
char *m_data;
struct list_member *m_next;
} MEMBER;

// initalise head pointer
MEMBER *headptr;

void load_string(char *newString)
{
printf("checkpoint1");
//create new list_member for new string
MEMBER *newmem, *curr;
printf("checkpoint2");
if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)
{
printf("checkpoint2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpoint2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen(newString)+1);
printf("checkpoint2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpoint2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0;
}
printf("checkpoint3");
// set current to 1st list_member
curr = headptr;
printf("checkpoint4");
// check if list is empty
if(curr->m_next == (MEMBER *)0)
{
printf("checkpoint4a");
headptr->m_next = newmem;
}
else
{
// go to the end of the list
printf("checkpoint4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpoint4c");
curr = curr->m_next;
}
printf("checkpoint5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpoint6");
}
}


then the testing program
test.c
#include "stringman.h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"123456789101112131415161718192021222324252627282930313233343536373839404
1424344454647484950515253545556575859606162636465666768697071727374757677
7879808182838485868788899091929394959697989910010110210310410510610710810
9110111112113114115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(test);
printf("load1ok");
load_string(test2);
printf("load2ok");
load_string(test3);
printf("load3ok");
printf("all loaded ok so it looks like");
}


and then the output


$ ./test

test = test1

test2 = test2

test3 =
12345678910111213141516171819202122232425262728293031323334353637383940
4142434445464748495051525354555657585960616263646566676869707172737475767
7787980
8182838485868788899091929394959697989910010110210310410510610710810911011
1112113
114115116117
Segmentation fault (core dumped)

as you can see, it seams to dump before any of the code in load_string()
is run.
im fucking confused as hell. like how can it dump before printing
checkpoint 1, its the 1st thing in string_load() so how is it dumping
before that...
 
N

nrk

deejaybags said:
could someone please have a look at this and tell me why it segfaults. i
am confused as all hell!

stringman.h
void load_string(char *newString);
char * remove_string();
char * remove_upper_string();
char * remove_lower_string();

(i havent written the removes yet);

then the source
stringman.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* fundamental structure definition */
typedef struct list_member{
char *m_data;
struct list_member *m_next;
} MEMBER;

// initalise head pointer
MEMBER *headptr;
Click to expand...

headptr is initialized alright. It's initialized to NULL, and
de-referencing it will produce undefined behavior (one manifestation of
which can be a "segfault").
void load_string(char *newString)
{
printf("checkpoint1");
Click to expand...
Read your C book carefully. The output stream can be line-buffered or
fully-buffered. Change this (and all your other printfs) to add a '\n' at
the end:
printf("checkpoint1\n");
If you're paranoid, add a fflush(stdout);. Then you should see atleast some
of your printf's showing up.
//create new list_member for new string
MEMBER *newmem, *curr;
printf("checkpoint2");
if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)
Click to expand...
Style Point: Way too much going on in one line. This style of coding might
make debugging difficult. Why not write:

newmem = malloc(sizeof *newmem);
if ( newmem == NULL )

Notice how the malloc has been done. This is the clc preferred way. Do
*NOT* cast the return of malloc. Use the pointer to be allocated to
determine the size for allocation, if possible.
{
printf("checkpoint2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpoint2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen(newString)+1);
Click to expand...
newmem->m_data = malloc(strlen(newString) + 1);

How about checking the return of that malloc as well? In the worst case,
atleast put an:
assert(ptr != NULL);
after your mallocs.
printf("checkpoint2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpoint2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0;
Click to expand...
What's wrong with:
newmem->m_next = NULL;
}
printf("checkpoint3");
// set current to 1st list_member
curr = headptr;
Click to expand...

Notice that headptr is still NULL and hasn't been initialized to be a valid
pointer to a MEMBER structure. Perhaps you missed:
if ( headptr == NULL ) {
headptr = newmem;
return;
}

That will make a list where the head is simply the first node of the list.

OTOH, if you wanted a linked list with a dummy head that is always present,
then you must allocate headptr as in:

MEMBER dummy;
MEMBER *headptr = &dummy;

instead of the declaration you have for headptr now.
printf("checkpoint4");
// check if list is empty
if(curr->m_next == (MEMBER *)0)
{
printf("checkpoint4a");
headptr->m_next = newmem;
}
else
{
// go to the end of the list
printf("checkpoint4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpoint4c");
curr = curr->m_next;
}
printf("checkpoint5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpoint6");
}
}


then the testing program
test.c
#include "stringman.h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"123456789101112131415161718192021222324252627282930313233343536373839404
1424344454647484950515253545556575859606162636465666768697071727374757677
7879808182838485868788899091929394959697989910010110210310410510610710810
9110111112113114115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(test);
printf("load1ok");
load_string(test2);
printf("load2ok");
load_string(test3);
printf("load3ok");
printf("all loaded ok so it looks like");
Click to expand...

return 0;

Turn up the warning levels in your compiler.

<OT>If you're using gcc, -Wall -ansi -O atleast.</OT>

Additional Style Point:
- Learn to intend your code consistently and in a manner that makes it easy
to read. If you lack ideas, turn to good C books or look at examples being
posted by the regulars here.

HTH,
nrk.
 
A

Artie Gold

deejaybags said:
could someone please have a look at this and tell me why it segfaults. i
am confused as all hell!
Click to expand...

In general, posting this much code is a Bad Idea. In the future,
limit your code to the smallest compilable snippet that exhibits the
problem (which has the added beneficial side effect of often
pointing you to the error in the process).

But OK, here goes...
stringman.h
void load_string(char *newString);
char * remove_string();
char * remove_upper_string();
char * remove_lower_string();

(i havent written the removes yet);

then the source
stringman.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* fundamental structure definition */
typedef struct list_member{
char *m_data;
struct list_member *m_next;
} MEMBER;

// initalise head pointer
MEMBER *headptr;

void load_string(char *newString)
{
printf("checkpoint1");
//create new list_member for new string
MEMBER *newmem, *curr;
printf("checkpoint2");
if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)
Click to expand...

Don't cast the return value of malloc(). It's unnecessary and can
hide other errors (like, for example, failing to include <stdlib.h>).

Also, it is better to use:
if ((newmem = malloc( sizeof *newmem) == NULL)

{
printf("checkpoint2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpoint2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen(newString)+1);
printf("checkpoint2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpoint2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0;
}
printf("checkpoint3");
// set current to 1st list_member
curr = headptr;
Click to expand...

Ding ding ding...
What's the value of `headptr' here?
[Hint: you neither initialize it nor assign to it]
printf("checkpoint4");
// check if list is empty
if(curr->m_next == (MEMBER *)0)
{
printf("checkpoint4a");
headptr->m_next = newmem;
}
else
{
// go to the end of the list
printf("checkpoint4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpoint4c");
curr = curr->m_next;
}
printf("checkpoint5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpoint6");
}
}


then the testing program
test.c
#include "stringman.h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"123456789101112131415161718192021222324252627282930313233343536373839404
1424344454647484950515253545556575859606162636465666768697071727374757677
7879808182838485868788899091929394959697989910010110210310410510610710810
9110111112113114115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(test);
printf("load1ok");
load_string(test2);
printf("load2ok");
load_string(test3);
printf("load3ok");
printf("all loaded ok so it looks like");
}


and then the output


$ ./test

test = test1

test2 = test2

test3 =
12345678910111213141516171819202122232425262728293031323334353637383940
4142434445464748495051525354555657585960616263646566676869707172737475767
7787980
8182838485868788899091929394959697989910010110210310410510610710810911011
1112113
114115116117
Segmentation fault (core dumped)

as you can see, it seams to dump before any of the code in load_string()
is run.
im fucking confused as hell. like how can it dump before printing
checkpoint 1, its the 1st thing in string_load() so how is it dumping
before that...
Click to expand...

It isn't. The stream stdout is typically buffered, hence output does
not appear unless you provide a newline character or explicitly
flush the stream.

HTH,
--ag
 
I

Irrwahn Grausewitz

deejaybags said:
could someone please have a look at this and tell me why it segfaults. i
am confused as all hell!

stringman.h
void load_string(char *newString);
char * remove_string();
char * remove_upper_string();
char * remove_lower_string();

(i havent written the removes yet);

then the source
stringman.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* fundamental structure definition */
typedef struct list_member{
char *m_data;
struct list_member *m_next;
} MEMBER;

// initalise head pointer
MEMBER *headptr;

void load_string(char *newString)
{
printf("checkpoint1");
//create new list_member for new string
MEMBER *newmem, *curr;
printf("checkpoint2");
if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)
Click to expand...
Better:
if ( ( newmem = malloc( sizeof *newmem ) ) == NULL )

But that's not the problem.
{
printf("checkpoint2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpoint2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen(newString)+1);
Click to expand...
Better:
newmem->m_data = malloc( strlen( newString ) + 1 );

You failed to check malloc()s return value.
But that's not the problem.
printf("checkpoint2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpoint2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0;
Click to expand...
Better:
newmem->m_next = NULL;
But that's still not the problem.
}
printf("checkpoint3");
// set current to 1st list_member
curr = headptr; Caaaaarefully...

printf("checkpoint4");
// check if list is empty
if(curr->m_next == (MEMBER *)0)
Click to expand...
Gotcha!!!
You failed to allocate some memory for headptr (and therefore curr)
to point to, so curr->m_next invokes dreaded undefined behaviour.
{
printf("checkpoint4a");
headptr->m_next = newmem;
}
Click to expand...
Change the above to:

// check if list is empty
if( headptr == NULL )
{
printf("checkpoint4a");
headptr = newmem;
}
else
{
// go to the end of the list
printf("checkpoint4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpoint4c");
curr = curr->m_next;
}
Click to expand...
That's a very inefficient way to do it. Maybe you want to maintain a
pointer to your last element, or just insert at the head of the list.
printf("checkpoint5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpoint6");
}
}


then the testing program
test.c
#include "stringman.h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"123456789101112131415161718192021222324252627282930313233343536373839404
1424344454647484950515253545556575859606162636465666768697071727374757677
7879808182838485868788899091929394959697989910010110210310410510610710810
9110111112113114115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(test);
printf("load1ok");
load_string(test2);
printf("load2ok");
load_string(test3);
printf("load3ok");
printf("all loaded ok so it looks like");
}


and then the output


$ ./test

test = test1

test2 = test2

test3 =
12345678910111213141516171819202122232425262728293031323334353637383940
4142434445464748495051525354555657585960616263646566676869707172737475767
7787980
8182838485868788899091929394959697989910010110210310410510610710810911011
1112113
114115116117
Segmentation fault (core dumped)

as you can see, it seams to dump before any of the code in load_string()
is run.
Click to expand...
Right, it /seems/ so, but: if you just had appended a '\n' to the
strings you print at your checkpoints (or did a fflush(stderr) after
each printf), you would have noticed that your program runs fine till
"checkpoint4" is printed, and then segfaults - reason given above.

im fucking confused as hell. like how can it dump before printing
checkpoint 1, its the 1st thing in string_load() so how is it dumping
before that...
Click to expand...

Relax, we've all gone through it. :)


Some more suggestions:
- don't use C99 single line comments when posting code here: it may
result in broken code if a line is wrapped by the newsreader, and
some people use compilers not capable to deal with //-comments

- do not cast the return value of malloc: you gain nothing but hiding
the fact you eventually forgot to #include <stdlib.h>

- just use NULL insted of (some_type *)0, that's what it is made for

- it's better style to write

my_ptr = malloc( sizeof *my_ptr * NUMBER );

instead of

my_ptr = malloc( sizeof (my_ptr_type) *NUMBER );

If the type of my_ptr ever changes in the future, you do not have to
go through your code and fix all the malloc()s.

Regards

Irrwahn
 
D

deejaybags

You guys are the sikest!!!! thanks heaps *punches self in head* had a
feeling the headptr was the prob but couldnt see the error with my
newbie brain! thanks again!! your all the best!!!!!!!!!!

peace
eye-bags-won
 
T

The Real OS/2 Guy

Style Point: Way too much going on in one line. This style of coding might
make debugging difficult. Why not write:
Click to expand...

Not a style point, a point to let the compier give a diagnostic when
you've forgotten to include stdlib.h. It helps you to debug your code
as it checks that you have the prototype known.

Never use a function that returns anything except int without the
prototype known.
newmem = malloc(sizeof *newmem);
if ( newmem == NULL )

Notice how the malloc has been done. This is the clc preferred way. Do
*NOT* cast the return of malloc. Use the pointer to be allocated to
determine the size for allocation, if possible.

newmem->m_data = malloc(strlen(newString) + 1);

How about checking the return of that malloc as well? In the worst case,
atleast put an:
assert(ptr != NULL);
Click to expand...

Never use assert to check data. assert is a debug function that will
either kill your program or even not active in productive code because
the compiler has the rights to eliminate it when not transating for
DEBUG is done.
assert is good to check for programming errors, but never for data
errors. a fail on lack of memory is mostenly not a cause the program
has to fail, it has at least to cleanup something (cleanup internal
data, save something to disk....) instead to break immediately with a
message the user does know about or interested on.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Why does spacing matter in this context? 0
URGENT 1
Why does strcat mess up the tokens in strtok (and strtok_r)? 92
SEGFAULT problem 5
Why not a redefinition? 3
Why does this compile OK, but fail to build? 2
Invalid write Valgrind 1
Structures and chained lists questions : 1

Members online

No members online now.
Total: 33 (members: 0, guests: 33)
Robots: 456

Forum statistics

Threads
473,769
Messages
2,569,580
Members
45,055
Latest member
SlimSparkKetoACVReview

Latest Threads

Top