Re: [patch] Improved error checking for the dovecot-antispam-plugin

[Robert Munteanu]

Hi,

Has this slipped off the radar or is it somehow not suitable for inclusion?

Thanks,

Robert

On Thu, Aug 18, 2016 at 6:16 PM, Robert Munteanu
 wrote:
> (snip)
>
>> I have no issue in resending a new version of the patch with better
>> error reporting, will do so in the following days.
>>
>> Robert
>
> I've attached a second version of the patch, feel free to consider any
> of them for inclusion.
>
> Thanks,
>
> Robert
>
>
> --
> http://robert.muntea.nu/



-- 
http://robert.muntea.nu/

Re: [patch] Improved error checking for the dovecot-antispam-plugin

[Harlan Stenn]

Robert,

First, thanks!

Second, I'm not a committer on the dovecot project. But I've written a lot of 
software where if an end user has a problem and either they want to know why or 
if they report it and ask for help, I've found it is MUCH better to have enough 
info in the message given to the user/logged somewhere. Something like:

"subroutine: open(%s) failed: %m"

It reduces our support load and gives us the information we need to quickly 
resolve issues. 

Sent from my iPhone - please excuse brevity and typos

> On Aug 18, 2016, at 8:16 AM, Robert Munteanu  
> wrote:
> 
> (snip)
> 
>> I have no issue in resending a new version of the patch with better
>> error reporting, will do so in the following days.
>> 
>> Robert
> 
> I've attached a second version of the patch, feel free to consider any
> of them for inclusion.
> 
> Thanks,
> 
> Robert
> 
> 
> -- 
> http://robert.muntea.nu/
> 

Re: [patch] Improved error checking for the dovecot-antispam-plugin

[Robert Munteanu]

(snip)

> I have no issue in resending a new version of the patch with better
> error reporting, will do so in the following days.
>
> Robert

I've attached a second version of the patch, feel free to consider any
of them for inclusion.

Thanks,

Robert


-- 
http://robert.muntea.nu/
changeset:   52:cce5dec86998
tag: tip
user:Robert Munteanu 
date:Fri Aug 12 15:33:12 2016 +0300
summary: Validate that mail_sendfile exists and is executable

diff -r 5ebc6aae4d7c -r cce5dec86998 src/mailtrain.c
--- a/src/mailtrain.c	Mon Apr 29 14:59:26 2013 +0200
+++ b/src/mailtrain.c	Fri Aug 12 15:33:12 2016 +0300
@@ -26,6 +26,7 @@
 #include "str.h"
 #include "istream.h"
 #include "ostream.h"
+#include "unistd.h"
 
 #include "aux.h"
 #include "backends.h"
@@ -107,6 +108,21 @@
 const char *dest = spam ? cfg->spam : cfg->non_spam;
 pid_t pid;
 int status;
+const char *err_msg;
+
+if (access(cfg->binary, F_OK) == -1)
+{
+err_msg = t_strdup_printf("Unable to read antispam_mail_sendfile file '%s'", cfg->binary);
+mail_storage_set_error(storage, MAIL_ERROR_TEMP, err_msg);
+return -1;
+}
+
+if (access(cfg->binary, X_OK) == -1)
+{
+err_msg = t_strdup_printf("Missing execute permissions on antispam_mail_sendfile file '%s'", cfg->binary);
+mail_storage_set_error(storage, MAIL_ERROR_TEMP, err_msg);
+return -1;
+}
 
 pid = fork();
 

Re: [patch] Improved error checking for the dovecot-antispam-plugin

[Robert Munteanu]

Hi Harlan,

On Wed, Aug 17, 2016 at 2:18 AM, Harlan Stenn  wrote:
> On 8/16/16 1:24 PM, Robert Munteanu wrote:
>> Hi,
>>
>> Hopefully this is the right channel for such a patch. I have a minor
>> enhancement to submit for the antispam plugin
>>
>>   http://hg.dovecot.org/dovecot-antispam-plugin
>>
>> It adds minimal error checking for the sendmail_binary, otherwise the
>> reported error in case of a missing binary or one with missing
>> permissions is generic and not useful.
>>
>> Thanks,
>>
>> Robert
>
> Robert, I like that you did this.
>
> Beyond that and without even looking at the actual code, I'm curious why
> you:
>
> +if (access(cfg->binary, F_OK) == -1)
> +{
> +mail_storage_set_error(storage, MAIL_ERROR_TEMP, "mail_sendmail
> file does not exist");
>
> instead of finding a way to include the value of cfg->binary in the
> error message string.
>
> This might not be needed if it's really obvious from the config file
> what the path to the executable is, but if there is any doubt it might
> be friendlier to show the exact path with the problem.  I'd also be
> inclined to show the decoded value of errno instead of assuming that
> 'mail_sendmail file does not exist'.
>
> Perhaps something along the lines of:
>
> "access(%s, F_OK) failed: %m", cfg->binary
>
> if that makes sense.

Thanks for the review . I was not sure that it's OK to show the path
to the script in an error message which will be shown to the user. But
I have no issue in resending a new version of the patch with better
error reporting, will do so in the following days.

Robert



-- 
http://robert.muntea.nu/

Re: [patch] Improved error checking for the dovecot-antispam-plugin

[Aki Tuomi]

On 17.08.2016 02:18, Harlan Stenn wrote:
> On 8/16/16 1:24 PM, Robert Munteanu wrote:
>> Hi,
>>
>> Hopefully this is the right channel for such a patch. I have a minor
>> enhancement to submit for the antispam plugin
>>
>>   http://hg.dovecot.org/dovecot-antispam-plugin
>>
>> It adds minimal error checking for the sendmail_binary, otherwise the
>> reported error in case of a missing binary or one with missing
>> permissions is generic and not useful.
>>
>> Thanks,
>>
>> Robert
> Robert, I like that you did this.
>
> Beyond that and without even looking at the actual code, I'm curious why
> you:
>
> +if (access(cfg->binary, F_OK) == -1)
> +{
> +mail_storage_set_error(storage, MAIL_ERROR_TEMP, "mail_sendmail
> file does not exist");
>
> instead of finding a way to include the value of cfg->binary in the
> error message string.
>
> This might not be needed if it's really obvious from the config file
> what the path to the executable is, but if there is any doubt it might
> be friendlier to show the exact path with the problem.  I'd also be
> inclined to show the decoded value of errno instead of assuming that
> 'mail_sendmail file does not exist'.
>
> Perhaps something along the lines of:
>
>   "access(%s, F_OK) failed: %m", cfg->binary
>
> if that makes sense.
>
> H
Hi!

Thank you for your patch, we'll take it under consideration!

Aki

Re: [patch] Improved error checking for the dovecot-antispam-plugin

[Harlan Stenn]

On 8/16/16 1:24 PM, Robert Munteanu wrote:
> Hi,
> 
> Hopefully this is the right channel for such a patch. I have a minor
> enhancement to submit for the antispam plugin
> 
>   http://hg.dovecot.org/dovecot-antispam-plugin
> 
> It adds minimal error checking for the sendmail_binary, otherwise the
> reported error in case of a missing binary or one with missing
> permissions is generic and not useful.
> 
> Thanks,
> 
> Robert

Robert, I like that you did this.

Beyond that and without even looking at the actual code, I'm curious why
you:

+if (access(cfg->binary, F_OK) == -1)
+{
+mail_storage_set_error(storage, MAIL_ERROR_TEMP, "mail_sendmail
file does not exist");

instead of finding a way to include the value of cfg->binary in the
error message string.

This might not be needed if it's really obvious from the config file
what the path to the executable is, but if there is any doubt it might
be friendlier to show the exact path with the problem.  I'd also be
inclined to show the decoded value of errno instead of assuming that
'mail_sendmail file does not exist'.

Perhaps something along the lines of:

"access(%s, F_OK) failed: %m", cfg->binary

if that makes sense.

H

[patch] Improved error checking for the dovecot-antispam-plugin

[Robert Munteanu]

Hi,

Hopefully this is the right channel for such a patch. I have a minor
enhancement to submit for the antispam plugin

  http://hg.dovecot.org/dovecot-antispam-plugin

It adds minimal error checking for the sendmail_binary, otherwise the
reported error in case of a missing binary or one with missing
permissions is generic and not useful.

Thanks,

Robert

-- 
http://robert.muntea.nu/
# HG changeset patch
# User Robert Munteanu 
# Date 1471005192 -10800
#  Fri Aug 12 15:33:12 2016 +0300
# Node ID 21a6999ba3d0ce670f790ce314d106bb9e23e337
# Parent  5ebc6aae4d7ce9ad4c53c1efe5b7f280e967d8a9
Validate that mail_sendfile exists and is executable

This prevents more cryptic errors from being raised to the users.

diff -r 5ebc6aae4d7c -r 21a6999ba3d0 src/mailtrain.c
--- a/src/mailtrain.c	Mon Apr 29 14:59:26 2013 +0200
+++ b/src/mailtrain.c	Fri Aug 12 15:33:12 2016 +0300
@@ -26,6 +26,7 @@
 #include "str.h"
 #include "istream.h"
 #include "ostream.h"
+#include "unistd.h"
 
 #include "aux.h"
 #include "backends.h"
@@ -108,6 +109,18 @@
 pid_t pid;
 int status;
 
+if (access(cfg->binary, F_OK) == -1)
+{
+mail_storage_set_error(storage, MAIL_ERROR_TEMP, "mail_sendmail file does not exist");
+return -1;
+}
+
+if (access(cfg->binary, X_OK) == -1)
+{
+mail_storage_set_error(storage, MAIL_ERROR_TEMP, "missing execute permissions on mail_sendmail");
+return -1;
+}
+
 pid = fork();
 
 if (pid == -1)