malloc/free mismatches in tradspool.c

[WATANABE Katsuhiro]

I've found some dangerous code portions in terms of
memory management in inn-2.4.1/storage/tradspool.c.
I'd be pleased if you can confirm following points
and fix them.

[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.

I know it works most cases but I believe the following code
would be cleaner and much safer :

   priv = *(PRIV_TRADSPOOL *) article->private;
   if (priv.artbase != NULL) {
       if (priv.mmapped)
           munmap(priv.artbase, priv.artlen);
       else
           free(priv.artbase);
   }
   free(article->private);
   free((void*)article);


[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.

I think, the for loop free-ing each elements should be
eliminated. You can find these for loop
    in tradspool_store()
        at line 592, 616, 626, 640, 660, 698, 709 and 721.
    in tradspool_cancel()
        at line 989.
    in tradspool_next()
        at line 1157.



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);

But I'am not sure which is the best way to handle the exceptional
case and how to fix that contradictions.
-- 
WATANABE Katsuhiro