Recommendation for remove check statement and inner logging statement under certain circumstances.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Recommendation for remove check statement and inner logging statement under certain circumstances.

牛旭
Our team researches on consistent update of Postfix during evolution. And we have figured out several spots that may be missed. 


By mining historical patches, we suggest that logging statement and corresponding control statement should be removed if the control statement validates whether return value of vstream_fseek() is smaller than 0.

One example of missed spot:


1  static const char *dict_mysql_lookup(DICT *dict, const char
  *name)
2  {
....

59 } else {
60 if (vstream_fseek(mp->fp, (off_t) 0, SEEK_END) < 0)
61 msg_fatal("%s: seek queue file %s: %m",
62 myname, VSTREAM_PATH(mp->fp));
63 mail_copy_status = mail_copy(COPY_ATTR(state.msg_attr), mp->fp,
64
....
}

One example of historical patch:
1  vstream_fclose(mp->fp);
2  dsb_simple(why, "4.2.0",
3  "destination %s is not owned by recipient", usr_attr.mailbox);
4  msg_warn("specify \"%s = no\" to ignore mailbox ownership  
  mismatch",
5  VAR_STRICT_MBOX_OWNER);
6  } else {
7  - if (vstream_fseek(mp->fp, (off_t) 0, SEEK_END) < 0)
8  -  msg_fatal("%s: seek queue file %s: %m",
9  -  myname, VSTREAM_PATH(mp->fp));
10 +  end = vstream_fseek(mp->fp, (off_t) 0, SEEK_END);
11  mail_copy_status = mail_copy(COPY_ATTR(state.msg_attr), mp->fp,
12  copy_flags, "\n", why);
13 }
14 mbox_release(mp);
15 }
16 set_eugid(var_owner_uid, var_owner_gid);

More recommendations and supporting patches are saved in attachments. It is so kind of you to reply me about the correctness of our suggestions. And thank you for your reading.

postfix-recommendation-example-2.doc (14K) Download Attachment
Postfix-patch-example-2.doc (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Recommendation for remove check statement and inner logging statement under certain circumstances.

Wietse Venema
??:
> Our team researches on consistent update of Postfix during evolution.
> And we have figured out several spots that may be missed.?
>

Postfix 3.2 adds a missing error check and logging message to the
virtual(8) delivery agent. This triggers when it fails to seek to
the end of a mailbox file. For unknown reasons, this change was not
made to the local(8) delivery agent which contains similar code.
That code is prsent in Postfix 1.0.0.

Your fix appears to remove the error check and logging message from
the virtual(8) delivery agent. I would recommend against that, and
instead add the error check and logging message the local(8) delivery
agent, as well as the virtual(8) delivery agent for older Postfix
versions.

Here is an example:

diff -ur /var/tmp/postfix-3.2.4/src/local/mailbox.c src/local/mailbox.c
--- /var/tmp/postfix-3.2.4/src/local/mailbox.c 2015-01-11 15:30:20.000000000 -0500
+++ src/local/mailbox.c 2018-01-06 10:52:53.000000000 -0500
@@ -97,7 +97,7 @@
     int     deliver_status;
     int     copy_flags;
     VSTRING *biff;
-    long    end;
+    off_t   end;
     struct stat st;
     uid_t   spool_uid;
     gid_t   spool_gid;
@@ -202,7 +202,8 @@
     msg_warn("specify \"%s = no\" to ignore mailbox ownership mismatch",
      VAR_STRICT_MBOX_OWNER);
  } else {
-    end = vstream_fseek(mp->fp, (off_t) 0, SEEK_END);
+    if ((end = vstream_fseek(mp->fp, (off_t) 0, SEEK_END)) < 0)
+ msg_fatal("%s: seek mailbox file %s: %m", myname, mailbox);
     mail_copy_status = mail_copy(COPY_ATTR(state.msg_attr), mp->fp,
  copy_flags, "\n", why);
  }

In the case of the virtual delivery agent, the length is never used
so the variable and assignment can be omitted.

        Wietse
Reply | Threaded
Open this post in threaded view
|

Re: Re: Recommendation for remove check statement and inner logging statement under certain circumstances.

牛旭



> -----Original Messages-----
> From: "Wietse Venema" <[hidden email]>
> Sent Time: 2018-01-07 00:15:36 (Sunday)
> To: "Postfix users" <[hidden email]>
> Cc:
> Subject: Re: Recommendation for remove check statement and inner logging statement under certain circumstances.
>
> ??:
> > Our team researches on consistent update of Postfix during evolution.
> > And we have figured out several spots that may be missed.?
> >
>
> Postfix 3.2 adds a missing error check and logging message to the
> virtual(8) delivery agent. This triggers when it fails to seek to
> the end of a mailbox file. For unknown reasons, this change was not
> made to the local(8) delivery agent which contains similar code.
> That code is prsent in Postfix 1.0.0.
>
> Your fix appears to remove the error check and logging message from
> the virtual(8) delivery agent. I would recommend against that, and
> instead add the error check and logging message the local(8) delivery
> agent, as well as the virtual(8) delivery agent for older Postfix
> versions.
>
> Here is an example:
>
> diff -ur /var/tmp/postfix-3.2.4/src/local/mailbox.c src/local/mailbox.c
> --- /var/tmp/postfix-3.2.4/src/local/mailbox.c 2015-01-11 15:30:20.000000000 -0500
> +++ src/local/mailbox.c 2018-01-06 10:52:53.000000000 -0500
> @@ -97,7 +97,7 @@
>      int     deliver_status;
>      int     copy_flags;
>      VSTRING *biff;
> -    long    end;
> +    off_t   end;
>      struct stat st;
>      uid_t   spool_uid;
>      gid_t   spool_gid;
> @@ -202,7 +202,8 @@
>      msg_warn("specify \"%s = no\" to ignore mailbox ownership mismatch",
>       VAR_STRICT_MBOX_OWNER);
>   } else {
> -    end = vstream_fseek(mp->fp, (off_t) 0, SEEK_END);
> +    if ((end = vstream_fseek(mp->fp, (off_t) 0, SEEK_END)) < 0)
> + msg_fatal("%s: seek mailbox file %s: %m", myname, mailbox);
>      mail_copy_status = mail_copy(COPY_ATTR(state.msg_attr), mp->fp,
>   copy_flags, "\n", why);
>   }
>
> In the case of the virtual delivery agent, the length is never used
> so the variable and assignment can be omitted.
>
> Wietse
that is to say, the patches that I found is against your design purpose.
So i agree with your opinion, thanks for reply.