Re: [exim-dev] Why does tls_dropprivs_validate_require_ciphe…

On 2012-12-15 at 00:15 +0100, Michael Haardt wrote:
> fflush(NULL) usually says "I got lost and try to cover that by saying
> I write robust code". There are ways to let this code work unmodified
> even on SunOS 4, like macros using wrapper functions, but to me it looks
> like a particularly bad place to get lost.

In this case, it was me saying "I want to minimise the risk of
disruption while adding new code, which has to be invoked as root, drop
privileges, and waited for, and impose as little disruption as
possible".

> As for logging, I see log_close_all() and prefer to see something like
> log_flush_all() being called, so it is easy to know what's going on there.
> I would have expected exim_exit() to be used in the child and having
> that call log_close_all(), instead fflush(NULL) and _exit() is used.
> If that is to avoid the side effects of exim_exit(), the child should
> rather free resources that must not be used first.

Actually, you might be highlighting a possible bug.

tls_dropprivs_validate_require_cipher() calls
tls_validate_require_cipher() which calls expand_check() upon
tls_require_ciphers, so Exim string expansion is invoked.

At which point, we possibly have databases opened, etc. So I think I
should have used exim_exit() here.

Which means I also now need to use search_tidyup() _before_ the fork(),
so that we don't have DB routine handles crossing parent/child usage
boundaries.

This is called fairly early in configuration file handling, so
comparatively few databases might already have been opened, so the risk
of a performance regression by closing the search DBs is small.

The other option would be to fork earlier, with a pipe, and write the
expanded string down the pipe from the parent at this point, so that the
child can validate it. This would require some re-architecting of the
TLS code to make feasible, but would avoid process/search/db
cross-contamination issues. I'll think on it some more.

-Phil