Problems with my little c-program

[Henk]

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* readFileToBuffe r(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(c har)),BUFFER_SI ZE,file);

if(current->size < BUFFER_SIZE){
if(ferror(file) != 0){
printf("file error, readFileToBuffe r()\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=create Buffer();
current->next = nextbuf;
current=current->next;
}
}
clearerr(file);
fclose(file);
return first;
}

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

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

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((*curren t).data);
printf("current->lenght=%d\n",c urrent->size);
current = (*current).next ;
}
printf((*curren t).data);
printf("current->lenght=%d\n",c urrent->size);
}
void reverseFile(str uct buffer *first){
printf("frst->lenght=%d\n",f irst->size);
struct buffer *current = first;
struct buffer *previousElm = NULL;
int laatsteElm;
int i;
char *kopie;

while(1){
if(current==NUL L){
break;
}
while(current->next != NULL){ //laatste buffer zoeken
previousElm=cur rent;
current=current->next;
printf("\na\n") ;
}
printf("\ncurre nt->lng=%d\n",curr ent->size);
printf("prev=%d \n",previousElm );
laatsteElm = current->size;
kopie=(current->data)+(curre nt->size)-1;
for(i=0;i < laatsteElm;i++) {
printf("%c",*ko pie);
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=readFile ToBuffer(filena me);
printf("firstsi ze=%d",first->size);
printNormalFile (first);
reverseFile(fir st);
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* readFileToBuffe r(char *filename);
struct buffer* createBuffer();
void printNormalFile (struct buffer *firstElem);
void reverseFile(str uct buffer *first);

#endif
-------------------------------------------------

[Richard Bos]

"Henk" <cc*****@hotmai l.com> 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(str uct buffer *first){
printf("frst->lenght=%d\n",f irst->size);
struct buffer *current = first;
struct buffer *previousElm = NULL;
int laatsteElm;
int i;
char *kopie;

while(1){
if(current==NUL L){
break;
}
while(current->next != NULL){ //laatste buffer zoeken
previousElm=cur rent;
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("\ncurre nt->lng=%d\n",curr ent->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)+(curre nt->size)-1;
for(i=0;i < laatsteElm;i++) {
printf("%c",*ko pie);
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

[Henk]

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)

[Michael Wojcik]

In article <11************ **********@f14g 2000cwb.googleg roups.com>, "Henk" <cc*****@hotmai l.com> 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 "previousEl m" 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(c har)),BUFFER_SI ZE,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(v oid){

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(size of(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(c har)*BUFFER_SIZ E);
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((*curren t).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==NUL L){
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 mi************@ microfocus.com

Please enjoy the stereo action fully that will surprise you. -- Pizzicato Five

[Henk]

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

Greetings Henk

[Barry Schwarz]

On 4 Oct 2005 05:02:25 -0700, "Henk" <cc*****@hotmai l.com> 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* readFileToBuffe r(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(str uct 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=readFile ToBuffer(filena me);
printf("firstsi ze=%d",first->size);
printNormalFile (first);
reverseFile(fir st);
return 0;
}
struct buffer* readFileToBuffe r(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(curren t->data,(sizeof(c har)),BUFFER_SI ZE,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, readFileToBuffe r()\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=create Buffer();
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(size of(struct buffer));
if (buf==NULL){
printf("Not possible to allocate memory\n");
exit(1);
}
buf->size = 0;
buf->next = NULL;
buf->data = malloc(sizeof(c har)*BUFFER_SIZ E);

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((*curren t).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",c urrent->size);
current = (*current).next ;
}
printf((*curren t).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",c urrent->size);
}
void reverseFile(str uct buffer *first){
printf("frst->lenght=%d\n",f irst->size);
struct buffer *current = first;
struct buffer *previousElm = NULL;
int laatsteElm;
int i;
char *kopie;

while(1){
if(current==NUL L){
break;
}
while(current->next != NULL){ //laatste buffer zoeken
previousElm=cur rent;
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("\ncurre nt->lng=%d\n",curr ent->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)+(curre nt->size)-1;
for(i=0;i < laatsteElm;i++) {
printf("%c",*ko pie);
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>>

[Zara]

On 4 Oct 2005 11:38:34 -0700, "Henk" <cc*****@hotmai l.com> 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.

[Michael Wojcik]

In article <43************ ***@news.xs4all .nl>, rl*@hoekstra-uitgeverij.nl (Richard Bos) writes:

"Henk" <cc*****@hotmai l.com> wrote:

void reverseFile(str uct buffer *first){
printf("frst->lenght=%d\n",f irst->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=cur rent;
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 mi************@ microfocus.com

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)

[Henk]

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
"segmentati on error"

The problem must be within this piece of code:

void reverseFile(str uct buffer *first){

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

int laatsteElm;
int i;
char *kopie;

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

laatsteElm = current->size;
kopie=(current->data)+(curre nt->size)-1;
for(i=0;i < laatsteElm;i++) {
printf("%c",*ko pie);
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

[Michael Wojcik]

In article <11************ *********@g44g2 000cwa.googlegr oups.com>, "Henk" <cc*****@hotmai l.com> 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
"segmentati on 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 "previousEl m". 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 "previousEl m->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 mi************@ microfocus.com

If Mokona means for us to eat this, I, a gentle person, will become
angry! -- Umi (CLAMP & unknown translator), _Magic Knight Rayearth_