Cyrus reviews

[Ken Murchison]

Ken Murchison wrote:
> Greg Banks wrote:

>> This code
>>
>> +    newdest = buf_release(&buf);
>>
>> will leak the string, as newdest is never free()d (and indeed in some 
>> other branches
>> of the logic, cannot be).  A better solution would be
>>
>>   const char *newdest = NULL;
>> ...
>>   newdest = buf_cstring(&buf);
>> ...
>>   buf_free(&buf);
>> }
> 
> I thought the same thing when I looked as what was already in master 
> when I began my forward port:
> 
>     replyto = buf_release(&buf);
>     if (body) {
>         /* replace the existing header */
>         spool_replace_header(xstrdup("Reply-To"), replyto,
>         m->hdrcache);
>     } else {
>         /* add the new header to the cache */
>         spool_cache_header(xstrdup("Reply-To"), replyto,
>         m->hdrcache);
>     }
> } else {
>     /* no newspostuser, use original replyto */
>     replyto = (char *) body[0];
> }
> 
> Was this also incorrect?

Actually, looking at your changes to spool.c (using strarray_t) and 
nntpd.c (using struct buf), I think that we DO want to use buf_release() 
because spool_cache_header() and spool_replace_header() both use 
strarray_appendm() to assign the existing pointer into the strarray 
(rather than making a copy) which is later free'd with strrarray_free() 
via spool_free_hdrcache().

Am I missing something?

-- 
Kenneth Murchison
Principal Systems Software Engineer
Carnegie Mellon University