On 19 March 2011 20:04, Jonny Mosco <[email protected]> wrote:
> I'm using smtpd as a smtp relay for around 50+ systems. It will run for a
> few days and eat up memory before crashing. The patch provided is by Jared
> Yanovich. After a good amount of testing, we discovered the memory leak.
>
> Index: asr.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/asr.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 asr.c
> --- asr.c 3 Dec 2010 23:29:08 -0000 1.2
> +++ asr.c 19 Mar 2011 19:48:33 -0000
> @@ -774,8 +774,7 @@ asr_ctx_query(struct asr_ctx *ac, int ty
> void
> asr_done(struct asr *asr)
> {
> - if (asr_ctx_unref(asr->a_ctx))
> - return;
> + asr_ctx_unref(asr->a_ctx);
> if (asr->a_path)
> free(asr->a_path);
> free(asr);
> Index: client.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/client.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 client.c
> --- client.c 28 Nov 2010 13:56:43 -0000 1.33
> +++ client.c 19 Mar 2011 19:48:33 -0000
> @@ -573,6 +573,7 @@ client_close(struct smtp_client *sp)
> SSL_free(sp->ssl);
> #endif
> close(sp->w.fd);
> + fclose(sp->body);
> free(sp);
> }
Client API's caller manages the message body descriptor so close()
belongs there. I believe the caller has been releasing this
descriptor correctly before this diff.
The problem is that, roughly speaking, fclose does free+close, while
what is needed here is just free. The solution is to use something
else than stdio, or to change the API so that caller no longer manages
the descriptor.
>
> @@ -690,6 +691,7 @@ client_getln(struct smtp_client *sp, int
>
> if (ln[3] == ' ')
> break;
> + free(ln);
> }
There are uses of ln after this free. I cannot see a case where ln
would not be freed.
>
> /* validate reply code */
> Index: queue_shared.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/queue_shared.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 queue_shared.c
> --- queue_shared.c 28 Nov 2010 14:35:58 -0000 1.35
> +++ queue_shared.c 19 Mar 2011 19:48:33 -0000
> @@ -124,6 +124,8 @@ queue_record_layout_envelope(char *queue
> FILE *fp;
> int fd;
>
> + fp = NULL;
> +
> again:
> if (! bsnprintf(evpname, sizeof(evpname), "%s/%s%s/%s.%qu",
> queuepath,
> message->message_id, PATH_ENVELOPES, message->message_id,
> @@ -154,14 +156,20 @@ again:
> fatal("queue_record_incoming_envelope: write");
> }
>
> - if (! safe_fclose(fp))
> + if (! safe_fclose(fp)) {
> + fp = NULL;
> + fd = -1;
> goto tempfail;
> + }
>
> return 1;
>
> tempfail:
> unlink(evpname);
> - close(fd);
> + if (fp)
> + fclose(fp);
> + else if (fd != -1)
> + close(fd);
> message->creation = 0;
> message->message_uid[0] = '\0';
