Unix Programming - what's wrong with this code

Author

what's wrong with this code

atv

2004-02-14, 11:33 pm

Hello,

I thought i was being smart and devised of this code after Jens helped me
out using fscanf for scanning with a delimiter. I changed it a bit and made
the code somewhat more compact and efficient to use less fscanf statements.

The code now scans for the | delimiter, increases len while fgetc doesn't
get a |, and then when it does, it uses that value to fread len amount of
characters.

I get a crash upon reading everything back, and i'm certain it is in here
somewhere. When running it trough gdb, it says that memcpy is the one being
complaining. So, that makes me to believe that one of the functions i'm
using (say, maybe strcpy) probably uses memcpy to perform it's function.

Could anyone maybe pinpoint me in the direction of what i'm doing wrong this
time ?

FILE *fp;
u_long len=0;
int i=0;
char del; /* Delimiter */
char *item[MAX_ITEM];

if((fp=fopen(IDX,"r"))==NULL) {
fprintf(stderr,"read_list_from_file: cannot open %s\n",IDX);
}
else {

/* Initialize the list */
for(i=0;i<MAX_ITEM;i++) item[i]=NULL;

/* Get all the items between delimiters */
i=0;
while(((del=fgetc(fp))!=EOF)) {
while((del=fgetc(fp))!='|') len++;
fseek(fp,ftell(fp)-len,SEEK_SET);
fread(item[i],len,1,fp);
*item[len]='\0';
len=0;
i++;
}

/* And now read them into our list, in proper order */
for(i=-1;i<MAX_ITEM;i++) {

if(tail==NULL) {
tail=malloc(sizeof(*tail));
head=tail;
} else {
tail->next=malloc(sizeof(*tail));
tail=(idx *)tail->next;
}

tail->band=strdup(item[i++]);
tail->album=strdup(item[i++]);
tail->mp3=strdup(item[i++]);
tail->size=strdup(item[i++]);
tail->upload=strdup(item[i++]);
tail->next=NULL;

if(item[i]==NULL) break;
}

fclose(fp);
printf("read_list_from_file: %s opened succesfully\n",IDX);
}

}

"read_list_from_file.c" 55L, 1085C

Jens.Toerring@physik.fu-berlin.de

2004-02-15, 12:33 am

atv <atv-nospam@xs4all.nl> wrote:
> The code now scans for the | delimiter, increases len while fgetc doesn't
> get a |, and then when it does, it uses that value to fread len amount of
> characters.

> I get a crash upon reading everything back, and i'm certain it is in here
> somewhere. When running it trough gdb, it says that memcpy is the one being
> complaining. So, that makes me to believe that one of the functions i'm
> using (say, maybe strcpy) probably uses memcpy to perform it's function.

gdb should tell you at what line in the code your program crashed. Did
you already find out about the 'backtrace' (or 'bt' for short) that
makes gdb tell you were it is and how it got there?

> Could anyone maybe pinpoint me in the direction of what i'm doing wrong this
> time ?

> FILE *fp;
> u_long len=0;
> int i=0;
> char del; /* Delimiter */
> char *item[MAX_ITEM];

> if((fp=fopen(IDX,"r"))==NULL) {
> fprintf(stderr,"read_list_from_file: cannot open %s\n",IDX);
> }
> else {

> /* Initialize the list */
> for(i=0;i<MAX_ITEM;i++) item[i]=NULL;

> /* Get all the items between delimiters */
> i=0;
> while(((del=fgetc(fp))!=EOF)) {

fgetc() returns an int and that's for a good reason: EOF isn't a char
but an int (usually, -1). When you store the return value of fgetc()
in a char you will never find out if you reached the end of the file
because the EOF gets truncated to something that looks like a valid char
but not like EOF anymore.

Another problem here is what happens if there are more entries in your
file than MAX_ITEM. You must check for this or you may use more elements
of 'item' then there are.

And what happens if the string you wrote into the file was empty and the
delimiter character is all there is for that string? You're not going
to find that out because you don't look at the first character. And
since you don't increment len for that first character you will always
skip the very first char of each string.

> while((del=fgetc(fp))!='|') len++;

I hope you realize that looking for the delimiter in the data requires
that the delimiter character is never going to be found in the strings
you are trying to read. Otherwise you're in for a surprise... It probably
would make more sense to use the '\0' char as your delimiter because
that can really never can appear in a string.

> fseek(fp,ftell(fp)-len,SEEK_SET);
> fread(item[i],len,1,fp);

item[i] is a NULL pointer (you explicitely initialized it to that). So
fread() tries to write what it just read from the file to a memory
location no program can ever access (and if you hadn't initialized all
the elements of item to NULL they would point to random locations in
memory which wouldn't make it better). That's were everthing crashes
(I can't find any use of memcopy() in your code and gdb wouldn't
complain about a function like that if you don't use, so you probably
have another bug in your program). You need to allocate len+1 chars to
item[i] before you can use fread() in that way (but then you won't need
the strdup() calls later).

BTW, have my doubts that reading the file twice, testing each char the
first time, is going to be much more efficient than having an ASCII
length before each string that tells you how many chars you need to read.

> *item[len]='\0';
> len=0;
> i++;
> }

> /* And now read them into our list, in proper order */
> for(i=-1;i<MAX_ITEM;i++) {

You shouldn't increment 'i' here, you're already doing that in the loop.
That way you're skipping every 6th element of 'item'.

> if(tail==NULL) {
> tail=malloc(sizeof(*tail));
> head=tail;
> } else {
> tail->next=malloc(sizeof(*tail));
> tail=(idx *)tail->next;

Why do you need a cast here?

> }

> tail->band=strdup(item[i++]);

Since you start with i set to -1 you need '++i' here, otherwise you're
going to try to access item[-1].

> tail->album=strdup(item[i++]);
> tail->mp3=strdup(item[i++]);
> tail->size=strdup(item[i++]);
> tail->upload=strdup(item[i++]);
> tail->next=NULL;

> if(item[i]==NULL) break;
> }

> fclose(fp);
> printf("read_list_from_file: %s opened succesfully\n",IDX);
> }

Regards, Jens
--
\ Jens Thoms Toerring ___ Jens.Toerring@physik.fu-berlin.de
\__________________________ http://www.toerring.de