why does this segfault??

[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(cha r *newString);
char * remove_string() ;
char * remove_upper_st ring();
char * remove_lower_st ring();

(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(cha r *newString)
{
printf("checkpo int1");
//create new list_member for new string
MEMBER *newmem, *curr;
printf("checkpo int2");
if((newmem=(MEM BER *)malloc(sizeof (MEMBER)))==(ME MBER *)0)
{
printf("checkpo int2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpo int2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen (newString)+1);
printf("checkpo int2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpo int2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0;
}
printf("checkpo int3");
// set current to 1st list_member
curr = headptr;
printf("checkpo int4");
// check if list is empty
if(curr->m_next == (MEMBER *)0)
{
printf("checkpo int4a");
headptr->m_next = newmem;
}
else
{
// go to the end of the list
printf("checkpo int4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpo int4c");
curr = curr->m_next;
}
printf("checkpo int5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpo int6");
}
}
then the testing program
test.c
#include "stringman. h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"12345678910111 213141516171819 202122232425262 728293031323334 3536373839404
142434445464748 495051525354555 657585960616263 646566676869707 1727374757677
787980818283848 586878889909192 939495969798991 001011021031041 0510610710810
911011111211311 4115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(tes t);
printf("load1ok ");
load_string(tes t2);
printf("load2ok ");
load_string(tes t3);
printf("load3ok ");
printf("all loaded ok so it looks like");
}
and then the output
$ ./test

test = test1

test2 = test2

test3 =
123456789101112 131415161718192 021222324252627 282930313233343 53637383940
414243444546474 849505152535455 565758596061626 364656667686970 7172737475767
7787980
818283848586878 889909192939495 969798991001011 021031041051061 0710810911011
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...

[Artie Gold]

deejaybags wrote:

could someone please have a look at this and tell me why it segfaults. i
am confused as all hell!
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(cha r *newString);
char * remove_string() ;
char * remove_upper_st ring();
char * remove_lower_st ring();

(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(cha r *newString)
{
printf("checkpo int1");
//create new list_member for new string
MEMBER *newmem, *curr;
printf("checkpo int2");
if((newmem=(MEM BER *)malloc(sizeof (MEMBER)))==(ME MBER *)0)
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("checkpo int2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpo int2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen (newString)+1);
printf("checkpo int2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpo int2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0;
}
printf("checkpo int3");
// set current to 1st list_member
curr = headptr;
Ding ding ding...
What's the value of `headptr' here?
[Hint: you neither initialize it nor assign to it]
printf("checkpo int4");
// check if list is empty
if(curr->m_next == (MEMBER *)0)
{
printf("checkpo int4a");
headptr->m_next = newmem;
}
else
{
// go to the end of the list
printf("checkpo int4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpo int4c");
curr = curr->m_next;
}
printf("checkpo int5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpo int6");
}
}
then the testing program
test.c
#include "stringman. h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"12345678910111 213141516171819 202122232425262 728293031323334 3536373839404
142434445464748 495051525354555 657585960616263 646566676869707 1727374757677
787980818283848 586878889909192 939495969798991 001011021031041 0510610710810
911011111211311 4115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(tes t);
printf("load1ok ");
load_string(tes t2);
printf("load2ok ");
load_string(tes t3);
printf("load3ok ");
printf("all loaded ok so it looks like");
}
and then the output
$ ./test

test = test1

test2 = test2

test3 =
123456789101112 131415161718192 021222324252627 282930313233343 53637383940
414243444546474 849505152535455 565758596061626 364656667686970 7172737475767
7787980
818283848586878 889909192939495 969798991001011 021031041051061 0710810911011
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...

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
--
Artie Gold -- Austin, Texas

[Irrwahn Grausewitz]

deejaybags <de********@yah oo.com> wrote:

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(cha r *newString);
char * remove_string() ;
char * remove_upper_st ring();
char * remove_lower_st ring();

(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(cha r *newString)
{
printf("checkpo int1");
//create new list_member for new string
MEMBER *newmem, *curr;
printf("checkpo int2");
if((newmem=(MEM BER *)malloc(sizeof (MEMBER)))==(ME MBER *)0) Better:
if ( ( newmem = malloc( sizeof *newmem ) ) == NULL )

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

You failed to check malloc()s return value.
But that's not the problem.
printf("checkpo int2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpo int2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0; Better:
newmem->m_next = NULL;
But that's still not the problem.
}
printf("checkpo int3");
// set current to 1st list_member
curr = headptr; Caaaaarefully.. .
printf("checkpo int4");
// check if list is empty
if(curr->m_next == (MEMBER *)0) Gotcha!!!
You failed to allocate some memory for headptr (and therefore curr)
to point to, so curr->m_next invokes dreaded undefined behaviour. {
printf("checkpo int4a");
headptr->m_next = newmem;
} Change the above to:

// check if list is empty
if( headptr == NULL )
{
printf("checkpo int4a");
headptr = newmem;
} else
{
// go to the end of the list
printf("checkpo int4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpo int4c");
curr = curr->m_next;
} 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("checkpo int5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpo int6");
}
}
then the testing program
test.c
#include "stringman. h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"1234567891011 121314151617181 920212223242526 272829303132333 43536373839404
14243444546474 849505152535455 565758596061626 364656667686970 71727374757677
78798081828384 858687888990919 293949596979899 100101102103104 10510610710810
91101111121131 14115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(tes t);
printf("load1ok ");
load_string(tes t2);
printf("load2ok ");
load_string(tes t3);
printf("load3ok ");
printf("all loaded ok so it looks like");
}
and then the output
$ ./test

test = test1

test2 = test2

test3 =
12345678910111 213141516171819 202122232425262 728293031323334 353637383940
41424344454647 484950515253545 556575859606162 636465666768697 07172737475767
7787980
81828384858687 888990919293949 596979899100101 102103104105106 10710810911011
1112113
114115116117
Segmentation fault (core dumped)

as you can see, it seams to dump before any of the code in load_string()
is run. 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
"checkpoint 4" 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...

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
--
I can't see it from here, but it looks good to me.

[nrk]

deejaybags wrote:

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(cha r *newString);
char * remove_string() ;
char * remove_upper_st ring();
char * remove_lower_st ring();

(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;

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(cha r *newString)
{
printf("checkpo int1"); 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("checkpo int1\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("checkpo int2");
if((newmem=(MEM BER *)malloc(sizeof (MEMBER)))==(ME MBER *)0) 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("checkpo int2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpo int2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen (newString)+1); newmem->m_data = malloc(strlen(n ewString) + 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("checkpo int2c");
/* copy data into structure, assuming it is a string */
strcpy(newmem->m_data, newString);
printf("checkpo int2d");
/* set the pointer in the structure to null */
newmem->m_next = (MEMBER *)0; What's wrong with:
newmem->m_next = NULL;
}
printf("checkpo int3");
// set current to 1st list_member
curr = headptr;
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("checkpo int4");
// check if list is empty
if(curr->m_next == (MEMBER *)0)
{
printf("checkpo int4a");
headptr->m_next = newmem;
}
else
{
// go to the end of the list
printf("checkpo int4b");
while(curr->m_next != (MEMBER *)0)
{
printf("checkpo int4c");
curr = curr->m_next;
}
printf("checkpo int5");
// change last ptr to point to current
curr->m_next = newmem;
printf("checkpo int6");
}
}
then the testing program
test.c
#include "stringman. h"

int main()
{
char *test = "test1", *test2= "test2", *test3 =
"12345678910111 213141516171819 202122232425262 728293031323334 3536373839404
142434445464748 495051525354555 657585960616263 646566676869707 1727374757677
787980818283848 586878889909192 939495969798991 001011021031041 0510610710810
911011111211311 4115116117";
printf("\n test = %s\n",test);
printf("\n test2 = %s\n",test2);
printf("\n test3 = %s\n",test3);
load_string(tes t);
printf("load1ok ");
load_string(tes t2);
printf("load2ok ");
load_string(tes t3);
printf("load3ok ");
printf("all loaded ok so it looks like");

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.

[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

[The Real OS/2 Guy]

On Thu, 11 Sep 2003 22:40:31 UTC, nrk <nr*******@hotm ail.com> wrote:

if((newmem=(MEM BER *)malloc(sizeof (MEMBER)))==(ME MBER *)0) Style Point: Way too much going on in one line. This style of coding might
make debugging difficult. Why not write:

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.

{
printf("checkpo int2a");
fprintf(stderr, "out of memory in new_member\n");
}
else
{
printf("checkpo int2b");
/* create memory to copy data into*/
newmem->m_data = (char *)malloc(strlen (newString)+1);

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

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

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.
--
Tschau/Bye
Herbert

eComStation 1.1 Deutsch Beta ist verügbar

[Irrwahn Grausewitz]

"The Real OS/2 Guy" <os****@pc-rosenau.de> wrote:

<SNIP>

Never use a function that returns anything except int without the
prototype known.

Never use any function without a prototype in scope.

<SNIP>

--
do not write: void main(...)
do not use gets()
do not cast the return value of malloc()
do not fflush( stdin )
read the c.l.c-faq: http://www.eskimo.com/~scs/C-faq/top.html