malloc/free mismatches in tradspool.c

[Russ Allbery]

WATANABE Katsuhiro <katsu at watanabe.name> writes:

> [1] Referring free-ed memory contents.

> In tradspool_next(), you can find following code:

>    1059         priv = *(PRIV_TRADSPOOL *) article->private;
>    1060         free(article->private);
>    1061         free((void*)article);
>    1062         if (priv.artbase != NULL) {
>    1063             if (priv.mmapped)
>    1064                 munmap(priv.artbase, priv.artlen);
>    1065             else
>    1066                 free(priv.artbase);
>    1067         }

> But at line 1062, structure priv(= article->private) has been
> free-ed already (at 1060). So theoretically speaking, it can
> happen to have garbage memory contents when its member is
> referred.

priv is not article->private, but rather a *copy* of the struct that
article->private had pointed to.  C doesn't make this particularly
obvious, unfortunately.  (Otherwise, the rest of the routine would also be
problematic, since it uses priv.)

> [2] Free-ing not allocated memory.

> You can find many code ideoms in tradspool.c saying:
>         xrefs = CrackXref(...);
>         ...
>         for (i = 0 ; i < numxrefs; ++i) free(xrefs[i]);
>         free(xrefs);      
> But the array xrefs is allocated as just ONE memory
> chunk, not as an array of allocated memories in
> CrackXref(). So freeing xref's each elements does not
> make any sense.

No, xrefs is allocated as an array of pointers to elements, and each one
of those elements is initialized separately.  See CrackXref; there's an
xmalloc at the top and then an xstrndup inside the loop.

> Also, in case CrackXref() returns NULL or 0-element array, there are
> some differences in the way the code handles the irregular case. Like:

> (A) Just free xrefs.
>     581     if ((xrefs = CrackXref(xrefhdr, &numxrefs)) == NULL || numxrefs == 0) {
>     584         if (xrefs != NULL)
>     585             free(xrefs);

> (B) Do nothing.
>     959     if ((xrefs = CrackXref(xrefhdr, &numxrefs)) == NULL || numxrefs == 0) {
>     960         free(path);
>     961         tradspool_freearticle(article);
>     962         SMseterror(SMERR_UNDEFINED, NULL);
>     963         return false;

> (C) Free xrefs and free its elements also.
>    1136             if ((xrefs = CrackXref(xrefhdr, &numxrefs)) == NULL || numxr
> efs == 0) {
>    1137                 art->len = 0;

>    /* Then falling through the if clause and it is handled later ... */

>    1157             for (i = 0 ; i < numxrefs ; ++i) free(xrefs[i]);
>    1158             free(xrefs);

(B) here is a bug (a minor memory leak).  I've fixed this in CVS.  The
other two cases are equivalent, since numxrefs will be zero.

-- 
Russ Allbery (rra at stanford.edu)             <http://www.eyrie.org/~eagle/>