somenath wrote, On 05/11/07 12:43:
Quote:
Hi All,
>
I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out some
improvement points or some error in the code.
>
Regards,
Somenath
>
char* get_line(FILE *fp)
{
int len = 2; /*First memory is allocated for 2 bytes 1 for one
character another for '\0'*/
int pos = 0;
char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);
}
You have a memory leak if fp was NULL and malloc succeeded. Test fp
before doing the malloc.
Quote:
while ( (c = fgetc(fp)) != EOF)
c should have been declared as an int rather than a char. If char is
unsigned this test will not detect EOF and if char is signed at least
one potentially valid character could be detected as EOF.
Quote:
{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos
but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;
One of "pos -= 1", "pos--" or "--pos" would be clearer in my opinion.
Quote:
/*We reached the end of the line*/
>
break;
}
if (pos == len)
{
>
len += 2;
temp_buff = realloc(buff,len);
if (temp_buff == NULL)
{
/*Reallocation of memory fails */
This is a memory leak since buf still points to allocated memory.
Quote:
return NULL;
}
else
{
>
buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);
This is another memory leak when the end of file is reached or a an
error occurs.
In general you should not throw away the successfully read data if an
error occurs. You should return what you have and also indicate that a
failure has occurred. It is also subject to a DoS (Denial of Service)
attack by feeding it a continuous stream or data that does not include a
new line.
--
Flash Gordon
