Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Problems with my little c-program

Reply
Thread Tools

Problems with my little c-program

 
 
Henk
Guest
Posts: n/a
 
      10-06-2005
Ok, just call me plain old stupid....but I really cannot see the
mistake.
I made a diagram and put assert() in the code to check the values of
current and previousElm and if they are NULL when they are supposed to
be. I feel really stupid but I just don't see it.
Could you tell me what I did wrong. I know for sure that I will learn
from my mistake.

Greetings Henk

 
Reply With Quote
 
 
 
 
Barry Schwarz
Guest
Posts: n/a
 
      10-07-2005
On 6 Oct 2005 07:09:02 -0700, "Henk" <(E-Mail Removed)> wrote:

>Hi Guy's,
>
>I worked on (almost) all the errors, but I am still getting a
>segmentation fault after running my program. We I just use 1 buffer
>everything seems to be going fine. But with more the one buffer (the
>program works...it reverses my file) but it ends u with giving me a
>"segmentation error"
>
>The problem must be within this piece of code:


If you would indent a reasonable amount, it would probably be obvious.
Indenting and bracing adjusted; view in a monospaced font.

Let's assume your linked list consists of two nodes. Let's assume the
first is at location 100 and the second at location 200.

>
>void reverseFile(struct buffer *first)
> {
>
> struct buffer *current = first;


Both current and first contain 100.

> struct buffer *previousElm = NULL;


previousElm contains 0.

>
> int laatsteElm;
> int i;
> char *kopie;
>
> while(1)
> {


Iterations refer to the while loop.

> if(current==NULL)
> {


On the first iteration, current is 100 (not NULL).
Same for second iteration.
Same for third iteration.

> break;


Not executed on iteration 1.
Same for iteration 2.
Same for iteration 3.

> }
> while(current->next != NULL)


True on iteration 1.
False on iteration 2.
False on iteration 3.

> { //laatste buffer zoeken
> previousElm=current;


On iteration 1 previousElm is assigned 100.

> current=current->next;


On iteration 1 current is assigned 200.

> }


Not executed on iteration 2. current and previousElm retain their
value of 100.
Same for iteration 3.

>
> laatsteElm = current->size;


On iteration 3, this invokes undefined behavior. You are
dereferencing current (100) which points to an area that has already
been freed.

> kopie=(current->data)+(current->size)-1;


Ditto.

> for(i=0;i < laatsteElm;i++)
> {
> printf("%c",*kopie);


Ditto.

> kopie--;
> }
> free(current->data);
> free(current);


On iteration 1, 200 and 200->data are freed.
On iteration 2, 100 and 100->data are freed.
On iteration 3, you try to free 100 and 100->data again. More
undefined behavior and the most likely place for a seg fault. This is
the same problem I told you about in the my last message.

> current=first;


On iteration 1, current is assigned 100.
Same for iteration 2.
I doubt if you get here for iteration 3.

> if(previousElm==NULL)


On iteration 1 this is false since previousElm is 100.
Same for iteration 2.

> {
> return;


Not executed on iteration 1.
Same for iteration 2.

> }
> previousElm->next=NULL;
>


Perform another iteration.

> }
>
> }
>
>Does anybody know what the problem is? I am getting a little bit
>creazy, I have been starring at my code for the last 3 hours, and did
>not know what the problem was.
>
>Greetings Henk



<<Remove the del for email>>
 
Reply With Quote
 
 
 
 
Michael Wojcik
Guest
Posts: n/a
 
      10-07-2005

In article <(E-Mail Removed) .com>, "Henk" <(E-Mail Removed)> writes:
> Ok, just call me plain old stupid....but I really cannot see the
> mistake.
> I made a diagram ...


I suspect the problem is that you're taking shortcuts when you
desk-check the code.

Don't just make a diagram. Make the diagram, then *walk through the
code*. Line by line. Update the diagram and the values you've
written down after every statement. Every time you dereference a
pointer, check to see that it's valid - that it's not null, that it
points to memory you've allocated, that the location hasn't been
freed. Every time you call free, check that the argument is valid.

If you make a diagram and then guess what the code will do with it,
you're just relying on your assumptions again. You have to actually
do what the code does, on paper. Be a C implementation.

> and put assert() in the code


No, no, no. You haven't finished with the diagram yet. You're
taking shortcuts that validate your assumptions. While assertions
can be a way to find bugs (though I dislike them myself), you're
using them as an excuse to not desk-check thoroughly.

I see Barry Schwarz has posted a detailed description of the
problem, so I won't. Basically, if your list starts off with two
or more elements, then when it gets down to one element previousElm
won't be updated anymore and you'll try to operate on nodes that
you've already freed.

--
Michael Wojcik http://www.velocityreviews.com/forums/(E-Mail Removed)

Pocket #9: A complete "artificial glen" with rocks, and artificial moon,
and forester's station. Excellent for achieving the effect of the
sublime without going out-of-doors. -- Joe Green
 
Reply With Quote
 
Dave Thompson
Guest
Posts: n/a
 
      10-10-2005
On 4 Oct 2005 17:11:39 GMT, (E-Mail Removed) (Michael Wojcik)
wrote:

>
> In article <(E-Mail Removed) .com>, "Henk" <(E-Mail Removed)> writes:

<snip>
> > struct buffer* createBuffer(){

<snip>
> Also, note that it's widely considered poor style to separate the
> "*" and the identifier for an identifier to pointer type. It leads
> to errors such as:
>
> int* ptrA, ptrB;
>
> where, contrary to its name, "ptrB" is actually an int, not an int
> pointer.
>

Beg to differ slightly. I agree it's usually a bad idea to join the *
with the type-specifiers, but that doesn't mean it should always be
joined to the name instead. E.g. I like:

int * ary_of_ptrs[10];
float * func_ret_ptr(argtypes);
/* or even */
float * func_ret_ptr (argtypes);

- David.Thompson1 at worldnet.att.net
 
Reply With Quote
 
Michael Wojcik
Guest
Posts: n/a
 
      10-10-2005

In article <(E-Mail Removed)>, Dave Thompson <(E-Mail Removed)> writes:
> On 4 Oct 2005 17:11:39 GMT, (E-Mail Removed) (Michael Wojcik)
> wrote:
> > Also, note that it's widely considered poor style to separate the
> > "*" and the identifier for an identifier to pointer type.
> >

> Beg to differ slightly. I agree it's usually a bad idea to join the *
> with the type-specifiers, but that doesn't mean it should always be
> joined to the name instead. E.g. I like:
>
> int * ary_of_ptrs[10];
> float * func_ret_ptr(argtypes);


I take your point. In a declaration like these for an identifer that
begins "type * identifier" (perhaps with storage class and/or
qualifier), but "*identifier" isn't an object of "type", separating
the "*" and the identifer can document that fact.

So:

float (*func_ptr)(argtypes);

for a pointer to a function returning float, but

float * func_ret_ptr(argtypes);

for a function returning a float pointer. That could indeed make the
code clearer on casual inspection (which is the main motivation for
whitespace, after all).

--
Michael Wojcik (E-Mail Removed)

Push up the bottom with your finger, it will puffy and makes stand up.
-- instructions for "swan" from an origami kit
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
1 little 2 little 3 little Kennedys dale Digital Photography 0 03-23-2008 01:03 PM
having a little problem with some code for a little game I am creating. ThaDoctor C++ 3 09-28-2007 03:28 PM
Gridview inside Gridview little problems... Carlos Albert ASP .Net 1 12-06-2005 05:31 PM
A little success but still problems =?Utf-8?B?TTc2?= Wireless Networking 14 01-18-2005 12:36 AM
little red X in little white box Puzzled Computer Support 8 12-13-2004 09:11 AM



Advertisments