Problems with my little c-program

Discussion in 'C Programming' started by Henk, Oct 4, 2005.

  1. Henk

    Henk Guest

    Hi Guys,

    (see actual code below)

    I wrote a little program that is supposed to read a file (input.txt)
    into memory (using a stack) and then reverse the file and display it to
    output. It works, but I still get some errors when i try to compile
    it:

    stack.c: In function 'reverseFile'
    stack.c:98: warning:int format, pointer arg (arg 2)

    And when I run the program It does reverse the input.txt file and
    displays it to output, but I am getting 2 more errors:

    free() invalid pointer 0x804a178!
    and in the end i get a:
    Segmentation fault.


    I know it is not the best C code ever, I am just getting started.I did
    some printf's in the code just for testing where it gets.

    I hope you can help me.

    Greetings Henk

    The code--------------------

    -----------stack.c-------------------
    #include <stdio.h>
    #include <stdlib.h>
    #include "stack.h"


    struct buffer* readFileToBuffer(char *filename){
    struct buffer *first;
    struct buffer *current;

    FILE *file =fopen(filename,"r");
    if(file==NULL){
    printf("Unable to open: %s\n", filename);
    exit(1);
    }

    first = createBuffer();
    current = first;

    while( !(feof(file))){ //VERANDERD
    //leest hele file in, maakt buffers aan en zet de file daarin
    current->size=
    fread(current->data,(sizeof(char)),BUFFER_SIZE,file);

    if(current->size < BUFFER_SIZE){
    if(ferror(file) != 0){
    printf("file error, readFileToBuffer()\n");
    clearerr(file);
    exit(1);
    }
    if(feof(file) !=0){
    printf("end of file\n");
    }
    }
    if(feof(file)==0){
    //alleen maken als feof nog niet gezet is. Anders overbodige
    buffer.
    struct buffer *nextbuf=createBuffer();
    current->next = nextbuf;
    current=current->next;
    }
    }
    clearerr(file);
    fclose(file);
    return first;
    }

    struct buffer* createBuffer(){
    struct buffer *buf;

    buf=malloc(sizeof(struct buffer));
    if (buf==NULL){
    printf("Not possible to allocate memory\n");
    exit(1);
    }
    buf->size = 0;
    buf->next = NULL;
    buf->data = malloc(sizeof(char)*BUFFER_SIZE);

    if (buf->data == NULL){
    printf("Not possible to allocate memory\n");
    exit(1);
    }
    return buf;
    }


    void printNormalFile (struct buffer *firstElem){
    struct buffer *current = firstElem;

    while((*current).next != NULL){
    printf((*current).data);
    printf("current->lenght=%d\n",current->size);
    current = (*current).next;
    }
    printf((*current).data);
    printf("current->lenght=%d\n",current->size);
    }


    void reverseFile(struct buffer *first){
    printf("frst->lenght=%d\n",first->size);
    struct buffer *current = first;
    struct buffer *previousElm = NULL;


    int laatsteElm;
    int i;
    char *kopie;

    while(1){
    if(current==NULL){
    break;
    }
    while(current->next != NULL){ //laatste buffer zoeken
    previousElm=current;
    current=current->next;
    printf("\na\n");
    }
    printf("\ncurrent->lng=%d\n",current->size);
    printf("prev=%d\n",previousElm);
    laatsteElm = current->size;
    kopie=(current->data)+(current->size)-1;
    for(i=0;i < laatsteElm;i++){
    printf("%c",*kopie);
    kopie--;
    }
    free(current->data);
    free(current);
    current=first;
    if(previousElm==NULL){
    return;
    }
    previousElm->next=NULL;

    }
    }

    int main(int argc, char** argv){

    char *filename=argv[1];

    struct buffer *first=readFileToBuffer(filename);
    printf("firstsize=%d",first->size);
    printNormalFile(first);
    reverseFile(first);


    return 0;
    }

    -------------------------------------------------------------------

    ---------stack.h---------------------------

    #ifndef STACK
    #define STACK

    #define BUFFER_SIZE 128
    #include <stdlib.h>

    struct buffer{
    char *data;
    struct buffer *next;
    int size;
    };

    struct buffer* readFileToBuffer(char *filename);
    struct buffer* createBuffer();
    void printNormalFile (struct buffer *firstElem);
    void reverseFile(struct buffer *first);

    #endif
    -------------------------------------------------
     
    Henk, Oct 4, 2005
    #1
    1. Advertising

  2. Henk

    Richard Bos Guest

    "Henk" <> wrote:

    > stack.c: In function 'reverseFile'
    > stack.c:98: warning:int format, pointer arg (arg 2)
    >
    > And when I run the program It does reverse the input.txt file and
    > displays it to output, but I am getting 2 more errors:
    >
    > free() invalid pointer 0x804a178!
    > and in the end i get a:
    > Segmentation fault.


    > void reverseFile(struct buffer *first){
    > printf("frst->lenght=%d\n",first->size);
    > struct buffer *current = first;
    > struct buffer *previousElm = NULL;
    >
    >
    > int laatsteElm;
    > int i;
    > char *kopie;
    >
    > while(1){
    > if(current==NULL){
    > break;
    > }
    > while(current->next != NULL){ //laatste buffer zoeken
    > previousElm=current;
    > current=current->next;
    > printf("\na\n");
    > }


    Note that this loop does not set previousElm if you have a one-node list
    (i.e., when current->next starts out null). Since this function breaks
    down the list, you will eventually encounter this situation. At that
    point, either previousElm will contain garbage (if first->next, as
    passed to the function, is null); or it will contain the value from the
    previous loop, which is then the same as current.

    > printf("\ncurrent->lng=%d\n",current->size);
    > printf("prev=%d\n",previousElm);


    This line causes the warning, since previousElm is a pointer (to a
    struct buffer, to be precise), and %d expects an int. To print a
    pointer, do this:

    printf("prev=%p\n", (void *)previousElm);

    (Since printf() is a variadic function, the cast to void * is important,
    btw.)

    > laatsteElm = current->size;
    > kopie=(current->data)+(current->size)-1;
    > for(i=0;i < laatsteElm;i++){
    > printf("%c",*kopie);
    > kopie--;
    > }
    > free(current->data);
    > free(current);
    > current=first;
    > if(previousElm==NULL){
    > return;
    > }
    > previousElm->next=NULL;


    I don't know why free() complains, but when previousElm == current, this
    line scribbles over memory that has already been free()d.

    Richard
     
    Richard Bos, Oct 4, 2005
    #2
    1. Advertising

  3. Henk

    Henk Guest

    Thanks for the info. I solved the "printf" problem thanks to your info.
    But I am still nog capable to solve the second problem:

    >Note that this loop does not set previousElm if you have a one-node list
    >(i.e., when current->next starts out null). Since this function breaks
    >down the list, you will eventually encounter this situation. At that
    >point, either previousElm will contain garbage (if first->next, as
    >passed to the function, is null); or it will contain the value from the
    >previous loop, which is then the same as current.


    and the following as mentioned by Richerd as wel:

    >when previousElm == current, this
    >line scribbles over memory that has already been free()d



    Could someone help me out, I 'm a Rooky in C programming (just started
    3 weeks ago). And was glad that I could make this more ore less work.
    But I cannot see what I should do to solve my problem.

    Greetings Henk (The C Rooky)
     
    Henk, Oct 4, 2005
    #3
  4. In article <>, "Henk" <> writes:
    >
    > stack.c: In function 'reverseFile'
    > stack.c:98: warning:int format, pointer arg (arg 2)


    This is the line in question:

    > printf("prev=%d\n",previousElm);


    and "previousElm" is a struct buffer *. %d is the format specifier
    for int arguments. A pointer is not an int. There are a number of
    ways to print the value of an object pointer; the simplest is to use
    the %p format specifier and cast the pointer to a void *:

    > printf("prev=%p\n", (void *)previousElm);


    > And when I run the program It does reverse the input.txt file and
    > displays it to output, but I am getting 2 more errors:
    >
    > free() invalid pointer 0x804a178!


    I recommend using a debugger. There are only two calls to free
    in the code you posted; it'd be trivial to insert a breakpoint
    and see what's happening at each call to free.

    (You'll need a list with at least two elements in it to see the
    error. Look at what values previousElm has if you make at least
    two iterations in the loop in reverseFile.)

    Some additional comments:

    > while( !(feof(file))){ //VERANDERD


    Don't do this; test the result of fread. See the comp.lang.c FAQ.

    > fread(current->data,(sizeof(char)),BUFFER_SIZE,file);


    sizeof(char) is always 1. And why the extra parentheses around it?

    > clearerr(file);
    > fclose(file);


    There's no point in calling clearerr on a FILE* that you're about
    to fclose.

    > struct buffer* createBuffer(){


    If a function takes no parameters, it should be declared with a
    parameter list of "void":

    struct buffer *createBuffer(void){

    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.

    > buf=malloc(sizeof(struct buffer));


    buf = malloc(sizeof *buf);

    is better; if the type of buf changes, the argument to malloc will
    still be correct.

    > buf->data = malloc(sizeof(char)*BUFFER_SIZE);


    Again, sizeof(char) is always 1.

    > while((*current).next != NULL){


    (*current).next is the same as current->next. That's the whole
    point of the arrow operator.

    > printf((*current).data);


    Bad idea. What if there's a percent-sign character in the data?
    This is a classic "format-string bug". Use one of:

    puts(current->data); /* this will append a newline */
    fputs(current->data, stdout); /* this will not */
    printf("%s\n", current->data); /* equivalent to puts */
    printf("%s", current->data); /* equivalent to fputs */

    Note there's no reason to prefer the printf alternatives. In
    general, use printf if you're formatting; to print only a single
    string, use puts/fputs.

    > while(1){
    > if(current==NULL){
    > break;
    > }


    while (current != NULL){

    has the same semantics and is clearer. However, you have another
    loop invariant buried in the loop:

    > if(previousElm==NULL){
    > return;
    > }


    and it's almost at the end of the loop (where while's control-
    expression would be evaluated), so it'd be even better to
    write:

    previousElm = current;
    while (current != NULL && previousElm != NULL){

    or the terser:

    previousElm = current;
    while (previousElm){

    since in this version, the only time current can be null is if
    reverseFile is called with a null pointer, and in that case
    previousElm will also be null on the first loop iteration.

    This assumes you've corrected the bug I alluded to above.

    > int main(int argc, char** argv){
    >
    > char *filename=argv[1];


    I realize this is only a learning exercise, but it's still a
    good idea to check whether you have an argv[1] before trying to
    use it. (And filename is extraneous here, though I suppose you
    might want to use it for the sake of clarity.)

    By the way, you're not using a stack here, in the conventional sense.
    You have a linked list, and when you reverse the file you're
    processing the list in reverse. The whole point of a stack is that
    each new element is added to the *front* of the stack, so that when
    you process the stack in order, you get the input in reverse.

    If you actually need to implement this with a stack, I'd recommend
    starting with the simplest implementation: one character per node,
    and use fgetc to read one character at a time. While it has
    tremendous overhead it would let you see exactly what's happening.
    Then when it's working you can optimize it.

    --
    Michael Wojcik

    Please enjoy the stereo action fully that will surprise you. -- Pizzicato Five
     
    Michael Wojcik, Oct 4, 2005
    #4
  5. Henk

    Henk Guest

    Thanks for the good explanation, it really helped.
    Hope to learn from my mistakes and get better.

    Greetings Henk
     
    Henk, Oct 4, 2005
    #5
  6. On 4 Oct 2005 05:02:25 -0700, "Henk" <> wrote:

    >Hi Guys,
    >
    >(see actual code below)
    >
    >I wrote a little program that is supposed to read a file (input.txt)
    >into memory (using a stack) and then reverse the file and display it to
    >output. It works, but I still get some errors when i try to compile
    >it:
    >
    >stack.c: In function 'reverseFile'
    >stack.c:98: warning:int format, pointer arg (arg 2)
    >
    >And when I run the program It does reverse the input.txt file and
    >displays it to output, but I am getting 2 more errors:
    >
    >free() invalid pointer 0x804a178!
    >and in the end i get a:
    >Segmentation fault.
    >
    >
    >I know it is not the best C code ever, I am just getting started.I did
    >some printf's in the code just for testing where it gets.
    >
    >I hope you can help me.


    I rearranged your functions so you can read the comments in
    chronological order.

    >
    >Greetings Henk
    >
    >The code--------------------
    >---------stack.h---------------------------
    >
    >#ifndef STACK
    >#define STACK
    >
    >#define BUFFER_SIZE 128
    >#include <stdlib.h>
    >
    >struct buffer{
    > char *data;
    > struct buffer *next;
    > int size;
    >};
    >
    >struct buffer* readFileToBuffer(char *filename);
    >struct buffer* createBuffer();


    It is better to specify void to explicitly state there are no
    arguments.

    >void printNormalFile (struct buffer *firstElem);
    >void reverseFile(struct buffer *first);
    >
    >#endif
    >-------------------------------------------------
    >
    >-----------stack.c-------------------
    >#include <stdio.h>
    >#include <stdlib.h>
    >#include "stack.h"
    >
    >int main(int argc, char** argv){
    >
    > char *filename=argv[1];


    You need to check that argc is at least 2.

    >
    > struct buffer *first=readFileToBuffer(filename);
    > printf("firstsize=%d",first->size);
    > printNormalFile(first);
    > reverseFile(first);
    >
    >
    >return 0;
    >}
    >
    >
    >struct buffer* readFileToBuffer(char *filename){
    > struct buffer *first;
    > struct buffer *current;
    >
    > FILE *file =fopen(filename,"r");
    > if(file==NULL){
    > printf("Unable to open: %s\n", filename);
    > exit(1);
    > }
    >
    > first = createBuffer();
    > current = first;


    first and current now both point to the same node. The node points to
    a 128 byte buffer which is uninitialized.

    >
    > while( !(feof(file))){ //VERANDERD
    > //leest hele file in, maakt buffers aan en zet de file daarin
    > current->size=
    >fread(current->data,(sizeof(char)),BUFFER_SIZE,file);


    This will read 128 bytes into the buffer without regard to the values.
    (Since you opened the file in text mode, it is possible that your
    system will intercept a certain value as an EOF indicator. Let's
    assume your file doesn't have any of these values.)

    Unfortunately, there is no reason to assume that any of the 128 values
    just read is '\0'. Consequently, the buffer does not contain a
    string.

    Were you perhaps thinking about using fgets?

    >
    > if(current->size < BUFFER_SIZE){
    > if(ferror(file) != 0){
    > printf("file error, readFileToBuffer()\n");
    > clearerr(file);
    > exit(1);
    > }
    > if(feof(file) !=0){
    > printf("end of file\n");
    > }
    > }
    > if(feof(file)==0){
    > //alleen maken als feof nog niet gezet is. Anders overbodige
    >buffer.
    > struct buffer *nextbuf=createBuffer();
    > current->next = nextbuf;
    > current=current->next;


    current now points to the next node in the list (which in turn points
    to a new buffer) and you continue reading. Unless your file is an
    exact multiple of 128 bytes, the last buffer will not be completely
    filled. If it is an exact multiple, the last buffer will be empty
    (actually completely uninitialized).

    > }
    > }
    > clearerr(file);
    > fclose(file);
    > return first;
    >}
    >
    >struct buffer* createBuffer(){
    > struct buffer *buf;
    >
    > buf=malloc(sizeof(struct buffer));
    > if (buf==NULL){
    > printf("Not possible to allocate memory\n");
    > exit(1);
    > }
    > buf->size = 0;
    > buf->next = NULL;
    > buf->data = malloc(sizeof(char)*BUFFER_SIZE);
    >
    > if (buf->data == NULL){
    > printf("Not possible to allocate memory\n");


    A different error message would be useful to distinguish between this
    error and the previous one.

    > exit(1);
    > }
    > return buf;
    >}
    >
    >
    >void printNormalFile (struct buffer *firstElem){
    > struct buffer *current = firstElem;
    >
    > while((*current).next != NULL){
    > printf((*current).data);


    You are telling printf to use the string in current buffer as a format
    string. Two problems:

    As noted above, it might not be a string. This would lead to
    undefined behavior.

    If the data in your file contains any '%', printf will treat it as
    a format specifier. Since you have no corresponding arguments in the
    argument list, this also would invoke undefined behavior. If the data
    in the buffer was a string, you would be better off with
    printf ("%s", current->data);
    or
    fputs(current->data, stdout);
    which would avoid this problem.

    Even though it doesn't seem like printf should ever change anything,
    once printf runs off the end of one of your 128 byte buffers the
    resulting undefined behavior could mess up malloc's internal control
    tables causing a subsequent free() to fail.

    > printf("current->lenght=%d\n",current->size);
    > current = (*current).next;
    > }
    > printf((*current).data);


    While not incorrect, your use of "(*current)." requires more typing
    and is idiomatically more obscure than the conventional "current->".

    > printf("current->lenght=%d\n",current->size);
    >}
    >
    >
    >void reverseFile(struct buffer *first){
    > printf("frst->lenght=%d\n",first->size);
    > struct buffer *current = first;
    > struct buffer *previousElm = NULL;
    >
    >
    > int laatsteElm;
    > int i;
    > char *kopie;
    >
    > while(1){
    > if(current==NULL){
    > break;
    > }
    > while(current->next != NULL){ //laatste buffer zoeken
    > previousElm=current;
    > current=current->next;
    > printf("\na\n");


    Did you really mean to print a bunch of double spaced lines, each with
    the same single character?

    > }


    After you have executed the while(1) loop n-1 times, there is only one
    node left. current, first, and previousElm all point to it. The
    above while loop never executes because current->next is NULL. Ten
    lines down you free it but then you come back to this point and ten
    lines later try to free it again. You cannot free the same area
    twice.

    > printf("\ncurrent->lng=%d\n",current->size);
    > printf("prev=%d\n",previousElm);


    previousELm is a pointer. Why are you printing out its value, which
    would be an address of no significance? Furthermore, %d requires an
    int value which previousElm is not. The only portable way to print a
    pointer is with %p and you must cast the value to void*.

    > laatsteElm = current->size;
    > kopie=(current->data)+(current->size)-1;
    > for(i=0;i < laatsteElm;i++){
    > printf("%c",*kopie);
    > kopie--;
    > }
    > free(current->data);
    > free(current);
    > current=first;
    > if(previousElm==NULL){


    Can this ever be true?

    > return;
    > }
    > previousElm->next=NULL;
    >
    > }
    >}
    >
    >-------------------------------------------------------------------
    >



    <<Remove the del for email>>
     
    Barry Schwarz, Oct 5, 2005
    #6
  7. Henk

    Zara Guest

    On 4 Oct 2005 11:38:34 -0700, "Henk" <> wrote:

    >Thanks for the good explanation, it really helped.
    >Hope to learn from my mistakes and get better.
    >
    >Greetings Henk



    Just two notes:


    One: It is considered good politics to quote the relevant parts of the
    message so that an accidental reader might now where all comes from.

    Two: I think I have seen the same problem statement of your original
    post as a question in other posting (from other person). This other
    person just wanted the full solution. You have been working a solution
    and, when it did not work in spite of all tyour tries, you asked for a
    limited, well oriente help. Well I *love* when newbiesdo this, that
    means they want to be programmers. Welcome.
     
    Zara, Oct 5, 2005
    #7
  8. In article <4all.nl>, (Richard Bos) writes:
    > "Henk" <> wrote:
    >
    > > void reverseFile(struct buffer *first){
    > > printf("frst->lenght=%d\n",first->size);


    I neglected to mention that in C90 variable declarations must preceed
    statements in a block, so this printf would have to be moved after
    the next two lines. In C99, declarations and statements can be
    mixed, and many pre-C99 compilers provide this as an extension, but
    it's a good idea to avoid it if you want portable code.

    > > struct buffer *current = first;
    > > struct buffer *previousElm = NULL;
    > >
    > > ...
    > >
    > > while(current->next != NULL){ //laatste buffer zoeken
    > > previousElm=current;
    > > current=current->next;
    > > printf("\na\n");
    > > }

    >
    > Note that this loop does not set previousElm if you have a one-node list
    > (i.e., when current->next starts out null). Since this function breaks
    > down the list, you will eventually encounter this situation. At that
    > point, either previousElm will contain garbage (if first->next, as
    > passed to the function, is null);


    Actually, in this case previousElm will be NULL, as it's initialized
    in the declaration above. The code should work if the original list
    is only one element long.

    > or it will contain the value from the
    > previous loop, which is then the same as current.


    But if the original list has two or more elements, then previousElm
    will indeed have a stale value.

    --
    Michael Wojcik

    I would never understand our engineer. But is there anything in this world
    that *isn't* made out of words? -- Tawada Yoko (trans. Margaret Mitsutani)
     
    Michael Wojcik, Oct 5, 2005
    #8
  9. Henk

    Henk Guest

    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:

    void reverseFile(struct buffer *first){

    struct buffer *current = first;
    struct buffer *previousElm = NULL;

    int laatsteElm;
    int i;
    char *kopie;

    while(1){
    if(current==NULL){
    break;
    }
    while(current->next != NULL){ //laatste buffer zoeken
    previousElm=current;
    current=current->next;
    }

    laatsteElm = current->size;
    kopie=(current->data)+(current->size)-1;
    for(i=0;i < laatsteElm;i++){
    printf("%c",*kopie);
    kopie--;
    }
    free(current->data);
    free(current);
    current=first;
    if(previousElm==NULL){
    return;
    }
    previousElm->next=NULL;

    }

    }

    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
     
    Henk, Oct 6, 2005
    #9
  10. In article <>, "Henk" <> writes:
    >
    > 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"


    Yes, that's one possible result of undefined behavior.

    > The problem must be within this piece of code:


    It's the same problem it had in the previous version of the program,
    and the same advice applies: look at what happens to previousElm if
    the list contains two elements.

    > I have been starring at my code for the last 3 hours, and did
    > not know what the problem was.


    Staring at code is rarely an effective debugging technique. To
    find a bug, you need to find which of your assumptions is wrong,
    and the way to do that is to test them.

    Here are some options:

    1. Use a debugger. Step through the function while it runs. Look
    at the values of the current and previousElm pointers, and at what
    values you pass to free.

    2. Take a piece of paper. Make a diagram of a two-element list,
    like this:

    +---+ +---+
    | | --> | | --> null
    +---+ +---+
    ^
    |
    first

    Take a couple of tokens - two distinct coins, perhaps - and call one
    "current" and one "previousElm". Looking at a listing of your
    function, walk through your function, moving the tokens to correspond
    to what they point to.

    When you free something, draw an X through it so you know it's been
    freed and that address is no longer valid. When you change a list
    pointer (eg "previousElm->next = NULL"), draw a new arrow in your
    diagram.

    You could also assign your own "addresses" to the list elements -
    just number them - and keep a list of which addresses have been
    passed to free on your paper.

    (Actually, you might want to try a three- or four-element list with
    this method, so that you can see what happens in both normal and
    boundary cases. The two-element case will show you your bug, if
    you do it correctly, but it won't show you why it doesn't happen on
    every iteration. That's useful to know when you go to fix it.)

    --
    Michael Wojcik

    If Mokona means for us to eat this, I, a gentle person, will become
    angry! -- Umi (CLAMP & unknown translator), _Magic Knight Rayearth_
     
    Michael Wojcik, Oct 6, 2005
    #10
  11. Henk

    Henk Guest

    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
     
    Henk, Oct 6, 2005
    #11
  12. On 6 Oct 2005 07:09:02 -0700, "Henk" <> 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>>
     
    Barry Schwarz, Oct 7, 2005
    #12
  13. In article <>, "Henk" <> 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

    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
     
    Michael Wojcik, Oct 7, 2005
    #13
  14. On 4 Oct 2005 17:11:39 GMT, (Michael Wojcik)
    wrote:

    >
    > In article <>, "Henk" <> 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
     
    Dave Thompson, Oct 10, 2005
    #14
  15. In article <>, Dave Thompson <> writes:
    > On 4 Oct 2005 17:11:39 GMT, (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

    Push up the bottom with your finger, it will puffy and makes stand up.
    -- instructions for "swan" from an origami kit
     
    Michael Wojcik, Oct 10, 2005
    #15
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. OZ
    Replies:
    2
    Views:
    1,446
  2. Richard Heathfield
    Replies:
    7
    Views:
    366
    Barry Schwarz
    Oct 5, 2003
  3. npc

    help with little program in C

    npc, Jan 11, 2004, in forum: C Programming
    Replies:
    38
    Views:
    872
    Mark McIntyre
    Jan 20, 2004
  4. ThaDoctor
    Replies:
    3
    Views:
    385
    Alan Woodland
    Sep 28, 2007
  5. Daniel
    Replies:
    1
    Views:
    214
    Bart van Ingen Schenau
    Jul 9, 2013
Loading...

Share This Page