[prev in list] [next in list] [prev in thread] [next in thread]
List: postfix-devel
Subject: [pfx-dev] Re: Submission service lookup support
From: Tomas Korbar via Postfix-devel <postfix-devel () postfix ! org>
Date: 2023-05-17 7:40:04
Message-ID: CABpWmcxW_vz2vTFaP67t9ZXMx5BzzSpC+K6Tp+Sq28B-VOeBtg () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Hi Wietse,
Thanks a lot for such a quick response and for these changes.
On Wed, May 17, 2023 at 1:50 AM Wietse Venema via Postfix-devel <
postfix-devel@postfix.org> wrote:
> Wietse Venema via Postfix-devel:
> > > There is an unnecessary double check of the "addr_list" pointer for
> null
> >
> > (twice). It does no harm, and a good compiler will remove that test.
> > Generally, I don't worry about repeated tests like this. They make
> > Postfix code a bit easier to maintain, because there are fewer
> > implicit dependencies.
> >
> > > In posttls-finger there are problems with using "service" char pointer
> in
> > > logging. I initialized the variable
> > > to null and ensured that it does not point into already freed space.
> >
> > Yeah, that is the result of clumsy memory management and should be
> fixed.
> >
> > I'll try to make this more obviously correct without adding multiple
> > instances of "if (bah != 0) myfree(bah);" throughout the code,
> > because it's too easy to forget adding another instance when the
> > code is updated.
>
> I simplified the code as with patch below. This 1) fixes the warning
> message when the destination in "dest" contains ":service" information,
> and 2) limits the visibility of some temporary variables.
>
> Wietse
>
> 20230517
>
> Bugfix (defect introduced: Postfix 3.8) the posttls-finger
> command could access uninitialized memory when reconnecting.
> Reported by Thomas Korbar. File: posttls-finger/posttls-finger.c.
>
> diff '--exclude=man' '--exclude=html' '--exclude=README_FILES'
> '--exclude=INSTALL' '--exclude=.indent.pro' -r -ur
> /var/tmp/postfix-3.9-20230516/src/posttls-finger/posttls-finger.c
> ./src/posttls-finger/posttls-finger.c
> --- /var/tmp/postfix-3.9-20230516/src/posttls-finger/posttls-finger.c
> 2023-04-13 10:16:06.000000000 -0400
> +++ ./src/posttls-finger/posttls-finger.c 2023-05-16
> 13:46:27.593011475 -0400
> @@ -1590,12 +1590,13 @@
> static void connect_remote(STATE *state, char *dest)
> {
> DNS_RR *addr;
> - char *buf;
> - char *domain;
> - char *service;
>
> /* When reconnecting use IP address of previous session */
> if (state->addr == 0) {
> + char *buf;
> + char *domain;
> + char *service;
> +
> buf = parse_destination(dest, state->smtp ? "smtp" : "24",
> &domain, &service, &state->port);
> if (!state->nexthop)
> @@ -1622,8 +1623,8 @@
>
> if (level == TLS_LEV_INVALID
> || (state->stream = connect_addr(state, addr)) == 0) {
> - msg_info("Failed to establish session to %s:%s via %s:%u: %s",
> - dest, service, HNAME(addr), addr->port,
> + msg_info("Failed to establish session to %s via %s:%u: %s",
> + dest, HNAME(addr), addr->port,
> vstring_str(state->why->reason));
> continue;
> }
> _______________________________________________
> Postfix-devel mailing list -- postfix-devel@postfix.org
> To unsubscribe send an email to postfix-devel-leave@postfix.org
>
>
[Attachment #5 (text/html)]
<div dir="ltr"><div>Hi Wietse,</div><div>Thanks a lot for such a quick response and \
for these changes.<br></div></div><br><div class="gmail_quote"><div dir="ltr" \
class="gmail_attr">On Wed, May 17, 2023 at 1:50 AM Wietse Venema via Postfix-devel \
<<a href="mailto:postfix-devel@postfix.org">postfix-devel@postfix.org</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Wietse Venema via \
Postfix-devel:<br> > > There is an unnecessary double check of the \
"addr_list" pointer for null<br> > <br>
> (twice). It does no harm, and a good compiler will remove that test.<br>
> Generally, I don't worry about repeated tests like this. They make<br>
> Postfix code a bit easier to maintain, because there are fewer<br>
> implicit dependencies.<br>
> <br>
> > In posttls-finger there are problems with using "service" char \
pointer in<br> > > logging. I initialized the variable<br>
> > to null and ensured that it does not point into already freed space.<br>
> <br>
> Yeah, that is the result of clumsy memory management and should be fixed. <br>
> <br>
> I'll try to make this more obviously correct without adding multiple<br>
> instances of "if (bah != 0) myfree(bah);" throughout the code,<br>
> because it's too easy to forget adding another instance when the<br>
> code is updated.<br>
<br>
I simplified the code as with patch below. This 1) fixes the warning<br>
message when the destination in "dest" contains ":service" \
information,<br> and 2) limits the visibility of some temporary variables.<br>
<br>
Wietse<br>
<br>
20230517<br>
<br>
Bugfix (defect introduced: Postfix 3.8) the posttls-finger<br>
command could access uninitialized memory when reconnecting.<br>
Reported by Thomas Korbar. File: posttls-finger/posttls-finger.c.<br>
<br>
diff '--exclude=man' '--exclude=html' \
'--exclude=README_FILES' '--exclude=INSTALL' '--exclude=.<a \
href="http://indent.pro" rel="noreferrer" target="_blank">indent.pro</a>' -r -ur \
/var/tmp/postfix-3.9-20230516/src/posttls-finger/posttls-finger.c \
./src/posttls-finger/posttls-finger.c<br>
--- /var/tmp/postfix-3.9-20230516/src/posttls-finger/posttls-finger.c 2023-04-13 \
10:16:06.000000000 -0400<br>
+++ ./src/posttls-finger/posttls-finger.c 2023-05-16 13:46:27.593011475 \
-0400<br> @@ -1590,12 +1590,13 @@<br>
static void connect_remote(STATE *state, char *dest)<br>
{<br>
DNS_RR *addr;<br>
- char *buf;<br>
- char *domain;<br>
- char *service;<br>
<br>
/* When reconnecting use IP address of previous session */<br>
if (state->addr == 0) {<br>
+ char *buf;<br>
+ char *domain;<br>
+ char *service;<br>
+<br>
buf = parse_destination(dest, state->smtp ? "smtp" : \
"24",<br>
&domain, &service, \
&state->port);<br> if (!state->nexthop)<br>
@@ -1622,8 +1623,8 @@<br>
<br>
if (level == TLS_LEV_INVALID<br>
|| (state->stream = connect_addr(state, addr)) == 0) {<br>
- msg_info("Failed to establish session to %s:%s via %s:%u: \
%s",<br>
- dest, service, HNAME(addr), addr->port,<br>
+ msg_info("Failed to establish session to %s via %s:%u: \
%s",<br> + dest, HNAME(addr), addr->port,<br>
vstring_str(state->why->reason));<br>
continue;<br>
}<br>
_______________________________________________<br>
Postfix-devel mailing list -- <a href="mailto:postfix-devel@postfix.org" \
target="_blank">postfix-devel@postfix.org</a><br> To unsubscribe send an email to <a \
href="mailto:postfix-devel-leave@postfix.org" \
target="_blank">postfix-devel-leave@postfix.org</a><br> <br>
</blockquote></div>
_______________________________________________
Postfix-devel mailing list -- postfix-devel@postfix.org
To unsubscribe send an email to postfix-devel-leave@postfix.org
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic