trouble with double free

[slashdotcommacolon]

Hello, I'm working on the exercises from k&r, exercise 5-13 is to
implement a simple replacement for the unix tail command. The brief
says it should be able to cope no matter how unreasonable the input and
should make best use of availbale storage (I suppose it means dont just
declare a big char array).

I have made a solution that seems to be basically working (tail -1
doesnt work, I'll try to fix that later), but I'm having trouble
free'ing allocated memory, if i add the call to free() where i thought
it should go my c library kills it (gnu c library) as it detects an
error.

If I just dont free the memory, it works, but I suppose that is bad
form.

I have added a comment where I thought I should be using free(), please
help me understand why this happens, as I am having trouble visualising
why it doesnt work.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define CHUNK_SIZE 256

char *getline(void);
void writeline(const char *line);

int main(int argc, char **argv)
{
int nlines = 10, end = 0, start = 1;
int i, cnt = 0;
char **linev, *line;

/* find number of lines required */
if (argc > 1 && *argv[1] == '-')
nlines = strtol(argv[1]+1, NULL, 10);

/* allocate space for nlines pointers */
if ((linev = malloc(sizeof (char *) * nlines)) == NULL)
return 1;

/* put each line into the last element of linev */
while ((line = getline()) != NULL) {
linev[end = (end + 1 == nlines) ? 0 : (end + 1)] = line;

if (++cnt >= nlines) {
/* this is where i would like to free(linev[start]); */
start = (start + 1 == nlines) ? 0 : (start + 1);
}
}

/* EOF received, print lines */

/* for nlines, or number of lines received if < nlines */
for (i = (cnt < nlines) ? cnt : nlines; i; i--) {
writeline(linev[start]);
free(linev[start]);
start = (start + 1 == nlines) ? 0 : (start + 1);
}

free (linev);

return 0;
}

char *getline(void)
{
char *line;
int c, n = 0;

if ((line = (char *) malloc(CHUNK_SIZE)) == NULL)
return line;

while ((c = getchar()) != EOF) {
if ((line[n++] = c) == '\n')
break;
if (CHUNK_SIZE - (n % CHUNK_SIZE) <= 2)
if ((line = (char *) realloc(line, ((n / CHUNK_SIZE + 1) *
CHUNK_SIZE) * sizeof (char))) == NULL)
return line;
}

line[n+1] = '\0';

if (n == 0)
free(line);

return (n) ? line : NULL;
}

void writeline(const char *line)
{
int i = 0;

for (i = strlen(line); i > 0; i--)
putchar (*(line++));
}

 

[Ulrich Eckhardt]

Hello, I'm working on the exercises from k&r, exercise 5-13 is to
implement a simple replacement for the unix tail command. The brief
says it should be able to cope no matter how unreasonable the input and
should make best use of availbale storage (I suppose it means dont just
declare a big char array).

I have made a solution that seems to be basically working (tail -1
doesnt work, I'll try to fix that later), but I'm having trouble
free'ing allocated memory, if i add the call to free() where i thought
it should go my c library kills it (gnu c library) as it detects an
error.

If I just dont free the memory, it works, but I suppose that is bad
form.

I have added a comment where I thought I should be using free(), please
help me understand why this happens, as I am having trouble visualising
why it doesnt work.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define CHUNK_SIZE 256

char *getline(void);

When I see this, I instantly wonder who owns the memory afterwards. Is it
some static/global string or is it perhaps allocated with malloc() and the
caller must free it?

void writeline(const char *line);

int main(int argc, char **argv)
{
int nlines = 10, end = 0, start = 1;
int i, cnt = 0;
char **linev, *line;

/* find number of lines required */
if (argc > 1 && *argv[1] == '-')
nlines = strtol(argv[1]+1, NULL, 10);

This lacks error checking. Also, in order to tail a file called '-10' it is
customary to use 'tail -- -10', i.e. use '--' to separate the arguments
from the options.

/* allocate space for nlines pointers */
if ((linev = malloc(sizeof (char *) * nlines)) == NULL)
return 1;

/* put each line into the last element of linev */
while ((line = getline()) != NULL) {
linev[end = (end + 1 == nlines) ? 0 : (end + 1)] = line;

IMHO too many expressions in this single line, I'd split it up for clarity:
linev[end] = line; // insert new line
end = (end+1)%nlines; // increment or wraparound (modulo operation!)

if (++cnt >= nlines) {
/* this is where i would like to free(linev[start]); */
start = (start + 1 == nlines) ? 0 : (start + 1);
}

Hmmm. I'd do it differently. Firstly, init linev with zeros. Then, before
entering an element into it, see if the place is already occupied and if
so, free() the former occupant. Also, while printing, check if an element
is occupied in the same way, that way you even eliminate the 'cnt'
fill-counter.

/* for nlines, or number of lines received if < nlines */
for (i = (cnt < nlines) ? cnt : nlines; i; i--) {
writeline(linev[start]);
free(linev[start]);
start = (start + 1 == nlines) ? 0 : (start + 1);
}

free (linev);

This last line is futile, just as releasing the lines after printing them
in the loop - some leak detectors might complain, but the program is
terminating so there's no need to free stuff, the OS will reclaim it
automatically.

char *getline(void)
{
char *line;
int c, n = 0;

if ((line = (char *) malloc(CHUNK_SIZE)) == NULL)
return line;

Oh, nono - never cast the returnvalue of malloc()(or realloc(), free(),
calloc() etc), it only serves to hide errors.

while ((c = getchar()) != EOF) {
if ((line[n++] = c) == '\n')
break;
if (CHUNK_SIZE - (n % CHUNK_SIZE) <= 2)
if ((line = (char *) realloc(line, ((n / CHUNK_SIZE + 1) *
CHUNK_SIZE) * sizeof (char))) == NULL)
return line;

It could be that it is the linebreaking newsreader, but I consider this
utterly unreadable. Remove the cast (see above) and the 'sizeof (char)'
which - by definition - is 1. Also, I'd have made CHUNK_SIZE a constant
inside that function, as it is only used there.

if (n == 0)
free(line);

return (n) ? line : NULL;

Hmm, obfuscating. You twice check for n==0 or n!=0 and act thereupon. Why
not a single one:
if(n==0)
{
free(line);
return NULL;
}
return line;

Uli

 

[Joe Wright]

Hello, I'm working on the exercises from k&r, exercise 5-13 is to
implement a simple replacement for the unix tail command. The brief
says it should be able to cope no matter how unreasonable the input and
should make best use of availbale storage (I suppose it means dont just
declare a big char array).

I have made a solution that seems to be basically working (tail -1
doesnt work, I'll try to fix that later), but I'm having trouble
free'ing allocated memory, if i add the call to free() where i thought
it should go my c library kills it (gnu c library) as it detects an
error.

With all due regard to BWK and the New Testament, tail is perhaps better
implemented another way:

Usage: tail [-n] <text>

Print the last n lines of <text> to stdout. Assume n defaults to 5.

It is not necessary to read lines at all. We read <text> with fgetc()
looking for '\n' and with ftell() determine the position in the file of
the next line until EOF.

Store the ftell() returns in an array n+1 of long configured as a ring.
At EOF, determine where EOF-n is, fseek() it and then fputc() until EOF
again.

 

[Walter Roberson]

With all due regard to BWK and the New Testament, tail is perhaps better
implemented another way:

Store the ftell() returns in an array n+1 of long configured as a ring.
At EOF, determine where EOF-n is, fseek() it and then fputc() until EOF
again.

That presumes that the input is seekable. Regular files are normally
seekable, but interactive terminals (or pipes or sockets) rarely are.

 

[Joe Wright]

That presumes that the input is seekable. Regular files are normally
seekable, but interactive terminals (or pipes or sockets) rarely are.

Thank you Walter. We don't usually concern ourselves here with pipes or
sockets. The C language has no such concepts. Do you suppose 'head' and
'tail' are meant to deal with other than files?

 

[Simon Biber]

Thank you Walter. We don't usually concern ourselves here with pipes or
sockets. The C language has no such concepts

That's true, but we usually don't concern ourselves with writing "head"
or "tail" commands, either. It all gets murky if you start talking about
implementing Unix tools but not implementing them 'properly'.

Do you suppose 'head' and
'tail' are meant to deal with other than files?

Absolutely, I use head and tail in pipe-lines all the time.

 

[Richard Bos]

Thank you Walter. We don't usually concern ourselves here with pipes or
sockets. The C language has no such concepts.

No, but we do concern ourselves with streams on which fseek() may fail;
a contingency for which the C Standard definitely does have a provision.

Do you suppose 'head' and 'tail' are meant to deal with other than files?

Definitely yes; I make use of this regularly, in fact more often than I
use it on a normal file.

Richard

 

[Dietmar Schindler]

Hello, I'm working on the exercises from k&r, exercise 5-13 is to
implement a simple replacement for the unix tail command. The brief
says it should be able to cope no matter how unreasonable the input and
should make best use of availbale storage (I suppose it means dont just
declare a big char array).

I have made a solution that seems to be basically working (tail -1
doesnt work, I'll try to fix that later), but I'm having trouble
free'ing allocated memory, if i add the call to free() where i thought
it should go my c library kills it (gnu c library) as it detects an
error.

If I just dont free the memory, it works, but I suppose that is bad
form.

I have added a comment where I thought I should be using free(), please
help me understand why this happens, as I am having trouble visualising
why it doesnt work.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define CHUNK_SIZE 256

char *getline(void);
void writeline(const char *line);

int main(int argc, char **argv)
{
int nlines = 10, end = 0, start = 1;
int i, cnt = 0;
char **linev, *line;

/* find number of lines required */
if (argc > 1 && *argv[1] == '-')
nlines = strtol(argv[1]+1, NULL, 10);

/* allocate space for nlines pointers */
if ((linev = malloc(sizeof (char *) * nlines)) == NULL)
return 1;

/* put each line into the last element of linev */
while ((line = getline()) != NULL) {
linev[end = (end + 1 == nlines) ? 0 : (end + 1)] = line;

if (++cnt >= nlines) {
/* this is where i would like to free(linev[start]); */

Here you would be freeing a line which you still want to output later.
For example nlines is 2, cnt is 2, end is 0, start is 1, stdin has 2
lines: you would free linev[1] and below call writeline with linev[1].
Suggestion: Drop the variable 'start' and free linev[end] at the
beginning of the loop after you incremented 'end' and before you assign
a /new/ line to it.

start = (start + 1 == nlines) ? 0 : (start + 1);
}
}

/* EOF received, print lines */

/* for nlines, or number of lines received if < nlines */
for (i = (cnt < nlines) ? cnt : nlines; i; i--) {
writeline(linev[start]);
free(linev[start]);
start = (start + 1 == nlines) ? 0 : (start + 1);
}

free (linev);

return 0;
}

char *getline(void)
{
char *line;
int c, n = 0;

if ((line = (char *) malloc(CHUNK_SIZE)) == NULL)
return line;

while ((c = getchar()) != EOF) {
if ((line[n++] = c) == '\n')
break;
if (CHUNK_SIZE - (n % CHUNK_SIZE) <= 2)
if ((line = (char *) realloc(line, ((n / CHUNK_SIZE + 1) *
CHUNK_SIZE) * sizeof (char))) == NULL)
return line;
}

line[n+1] = '\0';

if (n == 0)
free(line);

return (n) ? line : NULL;
}

void writeline(const char *line)
{
int i = 0;

for (i = strlen(line); i > 0; i--)
putchar (*(line++));
}