Three more patches

[Julien ÉLIE]

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)