Hi Tobias,
Tobias Stoeckmann wrote on Sun, Jul 12, 2015 at 02:35:18PM +0200:
> Sent this back in March, so maybe someone wants to review this time? :)
>
> tail -r has two memory leaks when handling non-regular files. You can
> easily see memory usage increasing with commands like
>
> $ mknod pipe p
> $ tail -r pipe pipe pipe > /dev/null
>
> And then writing a large file into the pipe three times. top shows the
> memory usage of tail increasing with each consecutive read.
>
> The obvious memory leak is at the end of reverse(): the doubly linked
> list (actually circular list) is never freed. To fix this, I break the
> circle and iterate through it, freeing items until I hit NULL.
>
> The less obvious one is in an error path. As tl->l is always of fixed
> size (BSZ), we can just change the struct to have a BSZ sized array in
> it. This removes the need to do checks in the error path completely.
Your analysis and fix look correct to me, and your patch
survived testing.
> While at it, there is no use to have a typedef in the code, so I just
> removed it, too.
That seems fine, too.
OK schwarze@.
Thanks,
Ingo
> Index: reverse.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/tail/reverse.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 reverse.c
> --- reverse.c 27 Oct 2009 23:59:44 -0000 1.19
> +++ reverse.c 12 Jul 2015 12:34:21 -0000
> @@ -147,12 +147,13 @@ r_reg(FILE *fp, enum STYLE style, off_t
> return (0);
> }
>
> -typedef struct bf {
> +#define BSZ (128 * 1024)
> +struct bf {
> struct bf *next;
> struct bf *prev;
> size_t len;
> - char *l;
> -} BF;
> + char l[BSZ];
> +};
>
> /*
> * r_buf -- display a non-regular file in reverse order by line.
> @@ -167,21 +168,19 @@ typedef struct bf {
> static void
> r_buf(FILE *fp)
> {
> - BF *mark, *tr, *tl = NULL;
> + struct bf *mark, *tr, *tl = NULL;
> int ch;
> size_t len, llen;
> char *p;
> off_t enomem;
>
> -#define BSZ (128 * 1024)
> for (mark = NULL, enomem = 0;;) {
> /*
> * Allocate a new block and link it into place in a doubly
> * linked list. If out of memory, toss the LRU block and
> * keep going.
> */
> - if (enomem || (tl = malloc(sizeof(BF))) == NULL ||
> - (tl->l = malloc(BSZ)) == NULL) {
> + if (enomem || (tl = malloc(sizeof(*tl))) == NULL) {
> if (!mark)
> err(1, NULL);
> tl = enomem ? tl->next : mark;
> @@ -259,5 +258,12 @@ r_buf(FILE *fp)
> while ((tl = tl->next)->len) {
> WR(tl->l, tl->len);
> tl->len = 0;
> + }
> +
> + tl->prev->next = NULL;
> + while (tl != NULL) {
> + tr = tl->next;
> + free(tl);
> + tl = tr;
> }
> }
