Three more patches

[Richard Kettlewell]

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