«

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 =
"1234567891011121314151617181920212223242526272829 30313233343536373839404
14243444546474849505152535455565758596061626364656 66768697071727374757677
78798081828384858687888990919293949596979899100101 10210310410510610710810
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 =
12345678910111213141516171819202122232425262728293 031323334353637383940
41424344454647484950515253545556575859606162636465 66676869707172737475767
7787980
81828384858687888990919293949596979899100101102103 10410510610710810911011
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 ****ing 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...

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

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");
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)
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);
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;
What's wrong with:
newmem->m_next = NULL;

> }
> printf("checkpoint3");
> // 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("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 =
> "1234567891011121314151617181920212223242526272829 30313233343536373839404
> 14243444546474849505152535455565758596061626364656 66768697071727374757677
> 78798081828384858687888990919293949596979899100101 10210310410510610710810
> 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");

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

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;

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 =
> "1234567891011121314151617181920212223242526272829 30313233343536373839404
> 14243444546474849505152535455565758596061626364656 66768697071727374757677
> 78798081828384858687888990919293949596979899100101 10210310410510610710810
> 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 =
> 12345678910111213141516171819202122232425262728293 031323334353637383940
> 41424344454647484950515253545556575859606162636465 66676869707172737475767
> 7787980
> 81828384858687888990919293949596979899100101102103 10410510610710810911011
> 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 ****ing 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

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(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)
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);
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;
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)
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;
> }
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;
> }
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 =
>"123456789101112131415161718192021222324252627282 930313233343536373839404
>1424344454647484950515253545556575859606162636465 666768697071727374757677
>7879808182838485868788899091929394959697989910010 110210310410510610710810
>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 =
>1234567891011121314151617181920212223242526272829 3031323334353637383940
>4142434445464748495051525354555657585960616263646 566676869707172737475767
>7787980
>8182838485868788899091929394959697989910010110210 310410510610710810911011
>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
"checkpoint4" is printed, and then segfaults - reason given above.


>im ****ing 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.

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

On Thu, 11 Sep 2003 22:40:31 UTC, nrk <> wrote:

> > if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)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("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);
> 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);

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

"The Real OS/2 Guy" <> 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