Re: [diff] src/usr.sbin/smtpd: plug two memory leaks

2020-12-17 Thread gilles
December 17, 2020 4:02 PM, gil...@poolp.org wrote:

> Hello,
> 
> The following diffs plug two memory leaks in smtpd:
> 
> a- in lka_filter.c, the name of the filter chain for a session is strdup()-ed
> upon session allocation but not released upon session release. free() it
> in lka_filter_end().
> 
> b- in smtp_session.c, filter io channel should be released when a tx is over,
> but it isn't. call io_free() on the channel in smtp_tx_free() if we do
> have a channel ready.
> 

please disregard b-, there's no leak here as martijn@ pointed out in private



Re: [diff] src/usr.sbin/smtpd: plug two memory leaks

2020-12-17 Thread Todd C . Miller
On Thu, 17 Dec 2020 15:02:41 +, gil...@poolp.org wrote:

> a- in lka_filter.c, the name of the filter chain for a session is strdup()-ed
> upon session allocation but not released upon session release. free() it
> in lka_filter_end().
>
> b- in smtp_session.c, filter io channel should be released when a tx is over,
> but it isn't. call io_free() on the channel in smtp_tx_free() if we do
> have a channel ready.
>
> diff --git a/usr.sbin/smtpd/lka_filter.c b/usr.sbin/smtpd/lka_filter.c
> index 9891e6140a3..6eb0829efca 100644
> --- a/usr.sbin/smtpd/lka_filter.c
> +++ b/usr.sbin/smtpd/lka_filter.c
> @@ -535,6 +535,7 @@ lka_filter_end(uint64_t reqid)
>   free(fs->mail_from);
>   free(fs->username);
>   free(fs->lastparam);
> + free(fs->filter_name);
>   free(fs);
>   log_trace(TRACE_FILTERS, "%016"PRIx64" filters session-end", reqid);
>  }

OK

> diff --git a/usr.sbin/smtpd/smtp_session.c b/usr.sbin/smtpd/smtp_session.c
> index 60123ad9a80..4530f44fb69 100644
> --- a/usr.sbin/smtpd/smtp_session.c
> +++ b/usr.sbin/smtpd/smtp_session.c
> @@ -2438,6 +2438,11 @@ smtp_tx_free(struct smtp_tx *tx)
>   if (tx->ofile)
>   fclose(tx->ofile);
>  
> + if (tx->filter) {
> + io_free(tx->filter);
> + tx->filter = NULL;
> + }
> +
>   tx->session->tx = NULL;
>  
>   free(tx);

Is there any reason to clear tx->filter when we are about to free tx?
If so, maybe tx->ofile should be cleared after close too.

 - todd