On 9/2/19 9:10 PM, [email protected] wrote: > September 2, 2019 7:29 PM, "Martijn van Duren" > <[email protected]> wrote: > >> On 9/2/19 6:00 PM, [email protected] wrote: >> >>> September 2, 2019 5:55 PM, [email protected] wrote: >>> >>>> September 2, 2019 5:23 PM, "Martijn van Duren" >>>> <[email protected]> wrote: >>> >>> Gilles should probably elaborate, but the way things are now is that >>> system(3) is used to start the filters, allowing us to run any arbitrary >>> (set of) command(s) as a filter. >>> >>> Since the filters now in ports are non-interactive commands I proposed >>> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent >>> of. This however means that all filters need to be specified by a full >>> path, which is not something I would promote. >>> >>> Hence the proposition of this diff. >>>> I don't feel comfortable adding that path to PATH, even if we're going >>>> to call system() right behind. >>>> >>>> Why not detect if the command starts with '/' and prepend libexec directory >>>> if that's not the case ? >>> >>> I also want to rework the command line before it's passed to system() so we >>> exec and save some unnecessary processes which are only waiting for a child >>> to exit its infinite loop. >>> >>> To do that, we are going to copy the command anyways so checking if command >>> starts with a / and prepending the absolute path is going to be easy. >> >> Something like this? >> > > can't test right now but comments inlined: > > >> Index: smtpd.c >> =================================================================== >> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v >> retrieving revision 1.324 >> diff -u -p -r1.324 smtpd.c >> --- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324 >> +++ smtpd.c 2 Sep 2019 17:27:31 -0000 >> @@ -1277,6 +1277,8 @@ fork_processor(const char *name, const c >> int sp[2], errfd[2]; >> struct passwd *pw; >> struct group *gr; >> + char exec[_POSIX_ARG_MAX]; > > I don't know if _POSIX_ARG_MAX is the proper define to use, > I genuinely don't know so someone more knowledgeable should jump in.
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html {_POSIX_ARG_MAX} Maximum length of argument to the exec functions including environment data. Value: 4 096 > > >> + int execr; >> >> if (user == NULL) >> user = SMTPD_USER; >> @@ -1340,6 +1342,14 @@ fork_processor(const char *name, const c >> signal(SIGHUP, SIG_DFL) == SIG_ERR) >> err(1, "signal"); >> >> + if (command[0] == '/') >> + execr = snprintf(exec, sizeof(exec), "exec %s", command); >> + else >> + execr = snprintf(exec, sizeof(exec), "exec %s/filter-%s", >> + PATH_LIBEXEC, command); > > I don't really like that 'filter-' is implicit. > > So if I specify full path it's filter-rspamd, when i don't it's rspamd, > which is confusing because that's also the name of a software on my > system. > > I think people can live with typing 'filter-' and we keep it explicit. Sure, no problem. I choose this approach to be more in line with queue-*, scheduler-*, and table-*. I'll change it before commit. > > > >> + if (execr >= (int) sizeof(exec)) >> + errx(1, "%s: exec path too long", name); >> + >> /* >> * Wait for lka to acknowledge that it received the fd. >> * This prevents a race condition between the filter sending an error >> @@ -1350,7 +1360,7 @@ fork_processor(const char *name, const c >> */ >> if (read(STDERR_FILENO, &buf, 1) != 0) >> errx(1, "lka didn't properly close write end of error socket"); >> - if (system(command) == -1) >> + if (system(exec) == -1) >> err(1, NULL); >> >> /* there's no successful exit from a processor */ >> Index: smtpd.conf.5 >> =================================================================== >> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v >> retrieving revision 1.221 >> diff -u -p -r1.221 smtpd.conf.5 >> --- smtpd.conf.5 17 Aug 2019 14:43:21 -0000 1.221 >> +++ smtpd.conf.5 2 Sep 2019 17:27:31 -0000 >> @@ -383,6 +383,11 @@ filter >> .Ar filter-name >> from >> .Ar command . >> +If >> +.Ar command >> +starts with a slash it is executed with an absolute path, >> +else it will be prepended with >> +.Dq /usr/local/libexec/smtpd/filter- . >> .It Ic include Qq Ar pathname >> Replace this directive with the content of the additional configuration >> file at the absolute >> @@ -757,6 +762,11 @@ Register an external process named >> from >> .Ar command . >> Such processes may be used to share the same instance between multiple >> filters. >> +If >> +.Ar command >> +starts with a slash it is executed with an absolute path, >> +else it will be prepended with >> +.Dq /usr/local/libexec/smtpd/filter- . >> .It Ic queue Cm compression >> Store queue files in a compressed format. >> This may be useful to save disk space.
