Send inn-workers mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.isc.org/mailman/listinfo/inn-workers
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of inn-workers digest..."
Today's Topics:
1. Today's patches (Richard Kettlewell)
2. Three more patches (Richard Kettlewell)
3. Re: Three more patches (Julien ?LIE)
4. Re: Three more patches (Richard Kettlewell)
5. INN and bug-finding tools (Richard Kettlewell)
6. Re: Today's patches (Julien ?LIE)
7. Re: Three more patches (Julien ?LIE)
----------------------------------------------------------------------
Message: 1
Date: Sat, 13 Jun 2015 14:05:50 +0100
From: Richard Kettlewell <[email protected]>
To: [email protected]
Subject: Today's patches
Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
Three more patches attached.
Has any thought been given to switching INN to the use of a more modern
version control system? svn is a bit inconvenient for non-committers;
as may be able to tell I'm using git's SVN integration to work around this.
ttfn/rjk
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-ninpaths-memory-leak.patch
Type: text/x-patch
Size: 1998 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20150613/4ada633e/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ident-avoid-buffer-underrun.patch
Type: text/x-patch
Size: 610 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20150613/4ada633e/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-innfeed-be-more-conservative-about-article-read-from.patch
Type: text/x-patch
Size: 1027 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20150613/4ada633e/attachment-0005.bin>
------------------------------
Message: 2
Date: Sat, 13 Jun 2015 19:59:16 +0100
From: Richard Kettlewell <[email protected]>
To: [email protected]
Subject: Three more patches
Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
The first and last fix actual bugs; the middle is really just
noise-reduction.
ttfn/rjk
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-innfeed-avoid-underrun-of-endpoint-array.patch
Type: text/x-patch
Size: 800 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20150613/08fe91a8/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-innfeed-innd-fix-boring-uninteresting-memory-leaks.patch
Type: text/x-patch
Size: 1817 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20150613/08fe91a8/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-nnrpd-don-t-strlcpy-with-equal-source-dest.patch
Type: text/x-patch
Size: 823 bytes
Desc: not available
URL:
<https://lists.isc.org/pipermail/inn-workers/attachments/20150613/08fe91a8/attachment-0005.bin>
------------------------------
Message: 3
Date: Sun, 14 Jun 2015 11:12:01 +0200
From: Julien ?LIE <[email protected]>
To: [email protected]
Cc: Richard Kettlewell <[email protected]>
Subject: Re: Three more patches
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
Hi Richard,
> The first and last fix actual bugs; the middle is really just
> noise-reduction.
Thanks again for your patches that contribute to make INN more robust.
> strlcpy with overlapping source & destination has undefined behavior.
> (INN's version of the function passes the source and destination to
> memcpy, which also has undefined behavior when they overlap.)
>
> diff --git a/nnrpd/post.c b/nnrpd/post.c
> index d6db59f..72515da 100644
> --- a/nnrpd/post.c
> +++ b/nnrpd/post.c
> @@ -1206,7 +1206,7 @@ ARTpost(char *article, char *idbuff, bool *permanent)
> return MailArticle(modgroup, article);
> }
>
> - if (idbuff)
> + if (idbuff && HDR(HDR__MESSAGEID) != idbuff)
> strlcpy(idbuff, HDR(HDR__MESSAGEID), SMBUF);
>
> if (PERMaccessconf->spoolfirst)
All right.
Then, shouldn't we also fix the same issue a few lines before?
--- nnrpd/post.c (r?vision 9880)
+++ nnrpd/post.c (copie de travail)
@@ -1155,8 +1155,9 @@
if (modgroup)
snprintf(idbuff, SMBUF, "(mailed to moderator for %s)",
modgroup);
- else
- strlcpy(idbuff, HDR(HDR__MESSAGEID), SMBUF);
+ else if (HDR(HDR__MESSAGEID) != idbuff) {
+ strlcpy(idbuff, HDR(HDR__MESSAGEID), SMBUF);
+ }
}
if (strncmp(p, "DROP", 4) == 0) {
syslog(L_NOTICE, "%s post failed %s", Client.host, p);
Strange that the static tool analyzer did not report it.
Maybe because it does not read all the code, depending on the preprocessor
variables enabled? (This code needs DO_PERL set.)
Wouldn't it be possible to parameter the static tool analyzer to analyze
several more paths of the code?
> --- a/innd/art.c
> +++ b/innd/art.c
> @@ -131,7 +131,7 @@ bool
> ARTreadschema(void)
> {
> const struct cvector *standardoverview;
> - const struct vector *extraoverview;
> + struct vector *extraoverview;
> unsigned int i;
> ARTOVERFIELD *fp;
> const ARTHEADER *hp;
> @@ -183,6 +183,7 @@ ARTreadschema(void)
> }
>
> fp->Header = NULL;
> + vector_free(extraoverview);
>
> return ok;
> }
Good catch!
Then I also suggest the following similar patches for makehistory and expire:
--- expire/makehistory.c (r?vision 9880)
+++ expire/makehistory.c (copie de travail)
@@ -417,7 +417,7 @@
ARTreadschema(bool Overview)
{
const struct cvector *standardoverview;
- const struct vector *extraoverview;
+ struct vector *extraoverview;
ARTOVERFIELD *fp;
unsigned int i;
@@ -474,6 +474,8 @@
}
ARTfieldsize = fp - ARTfields;
+ vector_free(extraoverview);
+
}
if (Bytesp == (ARTOVERFIELD *)NULL)
Missfieldsize++;
--- storage/expire.c (r?vision 9880)
+++ storage/expire.c (copie de travail)
@@ -539,7 +539,7 @@
ARTreadschema(void)
{
const struct cvector *standardoverview;
- const struct vector *extraoverview;
+ struct vector *extraoverview;
ARTOVERFIELD *fp;
unsigned int i;
@@ -566,6 +566,7 @@
}
ARTfieldsize = fp - ARTfields;
+ vector_free(extraoverview);
}
/*
> --- a/innfeed/host.c
> +++ b/innfeed/host.c
> @@ -2582,6 +2582,7 @@ void hostSetStatusFile (const char *filename)
> else if (*filename == '\0')
> die ("Can't set status file name with an empty string\n") ;
>
> + free(statusFile);
> if (*filename == '/')
> statusFile = xstrdup (filename) ;
> else {
The statusFile should normally be NULL at that time.
Unless we are in the case of several status-file: lines in innfeed.conf?
Is it the case that the patch fixes?
--
Julien ?LIE
? Il y a des gens qui parlent jusqu'? ce qu'ils arrivent ? trouver
quelque chose ? dire. ? (Sacha Guitry)
------------------------------
Message: 4
Date: Sun, 14 Jun 2015 10:32:36 +0100
From: Richard Kettlewell <[email protected]>
To: [email protected]
Subject: Re: Three more patches
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252
On 14/06/2015 10:12, Julien ?LIE wrote:
> Hi Richard,
>> strlcpy with overlapping source & destination has undefined behavior.
>> (INN's version of the function passes the source and destination to
>> memcpy, which also has undefined behavior when they overlap.)
>>
>> diff --git a/nnrpd/post.c b/nnrpd/post.c
>> index d6db59f..72515da 100644
>> --- a/nnrpd/post.c
>> +++ b/nnrpd/post.c
>> @@ -1206,7 +1206,7 @@ ARTpost(char *article, char *idbuff, bool *permanent)
>> return MailArticle(modgroup, article);
>> }
>>
>> - if (idbuff)
>> + if (idbuff && HDR(HDR__MESSAGEID) != idbuff)
>> strlcpy(idbuff, HDR(HDR__MESSAGEID), SMBUF);
>>
>> if (PERMaccessconf->spoolfirst)
>
> All right.
> Then, shouldn't we also fix the same issue a few lines before?
Oh, yes.
[...]
> Strange that the static tool analyzer did not report it.
> Maybe because it does not read all the code, depending on the preprocessor
> variables enabled? (This code needs DO_PERL set.)
> Wouldn't it be possible to parameter the static tool analyzer to analyze
> several more paths of the code?
These issues were identified through dynamic analysis (valgrind). My
test rig doesn't have very high coverage.
>> --- a/innfeed/host.c
>> +++ b/innfeed/host.c
>> @@ -2582,6 +2582,7 @@ void hostSetStatusFile (const char *filename)
>> else if (*filename == '\0')
>> die ("Can't set status file name with an empty string\n") ;
>>
>> + free(statusFile);
>> if (*filename == '/')
>> statusFile = xstrdup (filename) ;
>> else {
>
> The statusFile should normally be NULL at that time.
> Unless we are in the case of several status-file: lines in innfeed.conf?
> Is it the case that the patch fixes?
IIRC there's a new call to hostSetStatusFile() each time the config is
reloaded. There's also one in shutDown(), which I think is the path
valgrind saw.
ttfn/rjk
------------------------------
Message: 5
Date: Sun, 14 Jun 2015 10:35:30 +0100
From: Richard Kettlewell <[email protected]>
To: [email protected]
Subject: INN and bug-finding tools
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8
I think I'm reaching the point of diminishing returns with the Clang
analyzer, which is why I've switched strategy to valgrind for the time
being.
In the interests of avoiding duplicated work, has anyone else used any
of the following with INN:
- Coverity (they have a free service for free sw, which I've used on a
couple of my own projects)
- -fsanitize=... options to GCC/Clang
- afl-fuzz
ttfn/rjk
------------------------------
Message: 6
Date: Sun, 14 Jun 2015 12:07:51 +0200
From: Julien ?LIE <[email protected]>
To: [email protected]
Subject: Re: Today's patches
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi Richard,
> Three more patches attached.
Thanks, all committed for INN 2.6.0.
--
Julien ?LIE
? Il y a des gens qui parlent jusqu'? ce qu'ils arrivent ? trouver
quelque chose ? dire. ? (Sacha Guitry)
------------------------------
Message: 7
Date: Sun, 14 Jun 2015 12:12:44 +0200
From: Julien ?LIE <[email protected]>
To: [email protected]
Subject: Re: Three more patches
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi Richard,
> These issues were identified through dynamic analysis (valgrind). My
> test rig doesn't have very high coverage.
OK, thanks for your reply.
All your patches are now committed.
--
Julien ?LIE
? ? Et fais attention de ne pas te cogner aux arbres !
? ?
? Le tout, c'est de faire attention de ne pas se cogner aux
Gaulois ! ? (Ast?rix)
------------------------------
_______________________________________________
inn-workers mailing list
[email protected]
https://lists.isc.org/mailman/listinfo/inn-workers
End of inn-workers Digest, Vol 73, Issue 2
******************************************
