September 2, 2019 9:15 PM, "Martijn van Duren" <[email protected]> wrote:
> 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 > looks good to me unless someone objects >>> + 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. > okie dokie I dislike that we did this for queue-*, scheduler-* and table-* too, I'd rather see if we can change that in the future. I have a plan for 2020 to switch queue / scheduler / table to an API like filters' so we might want to revisit then :-) >>> + 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.
