Bug#533285: inn2: nnrpd crashes when retrieving entire thread

Julien ÉLIE julien at trigofacile.com
Tue Jun 16 19:10:15 UTC 2009


Hi Tim,

[Following a Debian bug report -- I put the inn-workers ml for discussion.]

> I have added NNTP-Posting-Host:full to my overview.fmt (and rebuilt all
> the databases)
> However there are some posts that do not have this header.
>
> When trying to retrieve all posts in a thread in slrn
> <esc>2<esc>p nnrpd crashes if there is a post in the thread that is
> missing this header.
>
> Jun 16 09:39:04 einstein nnrpd[15182]: failed to strndup 4294967278
> bytes at overdata.c line 414: Cannot allocate memory
>
> This is because the Header does not exist in the overview data for some
> posts but overview.c still tries to skip over it.

That's a nice catch.  Thanks!

LIST OVERVIEW.FMT
215 Order of fields in overview database
Subject:
From:
Date:
Message-ID:
References:
Bytes:
Lines:
Xref:full
Injection-Info:full
Newsgroups:full
.

GROUP trigofacile.test
211 203 6 227 trigofacile.test

OVER 227
224 Overview information for 227 follows
227     test      =?iso-8859-15?Q?Julien_=C9LIE?= <iulius at nom-de-mon-site.com.invalid>    Sun, 16 Jun 2009 13:39:17 +0100 
<gpisdvrdfud$7od2q1 at news.trigofacile.com>         788     1       Xref: news.trigofacile.com trigofacile.test:227 
Newsgroups: trigofacile.test
.

HDR Newsgroups 227
225 Header or metadata information for Newsgroups follows (from overview)
227 trigofacile.test
.

HDR Injection-Info 227
225 Header or metadata information for injection-info follows (from overview)
Connection closed by foreign host.


with the same error log as yours.




> --- inn2-2.4.5.orig/storage/overdata.c  2008-06-29 18:56:57.000000000 +0100
> +++ inn2-2.4.5/storage/overdata.c       2009-04-01 14:56:41.000000000 +0100
> @@ -6,6 +6,8 @@
> **  tab-separated list of overview fields.
> */
>
> +#include <syslog.h>
> +
> #include "config.h"
> #include "clibrary.h"
> #include <ctype.h>
> @@ -407,9 +409,26 @@
>        p = vector->strings[element] +
>            strlen(extra->strings[element - ARRAY_SIZE(fields)]) + 2;
>        len = vector->strings[element + 1] - p - 1;
> +        if(p>=vector->strings[element + 1])
> +          len=0;
>     field = xstrndup(p, len);
>     return field;

According to how overview_getheader() is used by nnrpd, the right answer
to give is:

+    if (len >= 0) {
         field = xstrndup(p, len);
+    } else {
         field = NULL;
+    }
     return field;

otherwise, it will return '\0', that is to say an empty string which
would imply that the header exists in the article and is empty.



However, there is a more fundamental bug in xstrndup() because it
does not check the validity of the size.

char *
x_strndup(const char *s, size_t size, const char *file, int line)
{
    char *p;

    p = malloc(size + 1);
    while (p == NULL) {
        (*xmalloc_error_handler)("strndup", size + 1, file, line);
        p = malloc(size + 1);
    }
    memcpy(p, s, size);
    p[size] = '\0';
    return p;
}

Russ, should it also be fixed here?
(This code is the same in rra-c-util 1.0.)



> note that I don't think this fix is strictly correct as the calculation
> of p can still run off the end of the array.

Do you have an example?
Or can it happen only when the overview database is inconsistent? (that is
to say when the last field is "short: a" and the overview.fmt list advertises
a header whose length is greater than the one of "short"?)

-- 
Julien ÉLIE

« Quand on sait que le pied vaut environ 33 cm et que l'alexandrin
  compte 12 pieds, il est facile de calculer qu'un stade vaut
  environ 42 alexandrins. » (Astérix) 




More information about the inn-workers mailing list