Skip to content
Permalink
Browse files

Fix bugs in "STARTTLS.... Okay, restart kid!"

Bug 1: Zombies. We were waiting for kid only in the original
teardown_and_exit() case, not in the new stop_kid() case.

Bug 2: Never actually re-exec()'d qmail-smtpd in start_kid(). Noticed by
following a refactoring suggested by Christoph Badura: exit the read
loop on STARTTLS, update process and pipe state, then reenter.

Still some duplication here.
  • Loading branch information...
schmonz committed Jan 31, 2019
1 parent e99b40b commit 5e57ce4db0079b4da361c64f18b5fdde9194e6e8
Showing with 49 additions and 38 deletions.
  1. +1 −0 fixsmtpio.h
  2. +47 −37 fixsmtpio_proxy.c
  3. +1 −1 fixsmtpio_proxy.h
@@ -22,6 +22,7 @@

#define RESPONSELINE_NOCHANGE "&" PROGNAME "_noop"

#define BEGIN_STARTTLS_NOW -2
#define EXIT_LATER_NORMALLY -1
#define EXIT_NOW_SUCCESS 0
#define EXIT_NOW_TEMPFAIL 14
@@ -287,49 +287,63 @@ static void adjust_proxy_for_new_kid(int from_proxy,int to_proxy,
eventq_put(EVENT_GREETING);
}

static void stop_kid(int kid_pid,int from_server,int to_server) {
/* XXX copypasta from teardown_and_exit() */
int wstat;

unistd_close(from_server);
unistd_close(to_server);

if (-1 == kill(kid_pid,SIGTERM)) die_kill();

if (wait_pid(&wstat,kid_pid) == -1) die_wait();
}

static int start_kid_with_pipes(int *from_proxy,int *to_server,
int *from_server,int *to_proxy) {
make_pipe(from_proxy,to_server);
make_pipe(from_server,to_proxy);
return unistd_fork();
}

static void be_proxy(int from_client,int to_client,
int from_proxy,int to_proxy,
int from_server,int to_server,
stralloc *greeting,filter_rule *rules,
int kid_pid,char **argv) {
int exitcode;
int in_tls = 0;
stralloc logstamp = {0};

adjust_proxy_for_new_kid(from_proxy,to_proxy,
&logstamp,
kid_pid,basename(argv[0]));

exitcode = read_and_process_until_either_end_closes(from_client,to_server,
from_server,to_client,
greeting,rules,
&logstamp,
&kid_pid,argv);
teardown_and_exit(exitcode,kid_pid,rules,from_server,to_server);
}

static void stop_kid(int kid_pid,int from_server,int to_server) {
unistd_close(from_server);
unistd_close(to_server);
if (-1 == kill(kid_pid,SIGTERM)) die_kill();
}

static void start_kid(int *kid_pid,int *from_server,int *to_server,
stralloc *logstamp,char **argv) {
int from_proxy, to_proxy;

make_pipe(&from_proxy,to_server);
make_pipe(from_server,&to_proxy);
&logstamp,in_tls);
if (exitcode == BEGIN_STARTTLS_NOW) {
stop_kid(kid_pid,from_server,to_server);
kid_pid = start_kid_with_pipes(&from_proxy,&to_server,&from_server,&to_proxy);
if (kid_pid) {
in_tls = 1;
adjust_proxy_for_new_kid(from_proxy,to_proxy,
&logstamp,
kid_pid,basename(argv[0]));
exitcode = read_and_process_until_either_end_closes(from_client,to_server,
from_server,to_client,
greeting,rules,
&logstamp,in_tls);
} else if (0 == kid_pid) {
be_proxied(from_proxy,to_proxy,
from_server,to_server,
argv);
} else {
die_fork();
}
}

if ((*kid_pid = unistd_fork()))
adjust_proxy_for_new_kid(from_proxy,to_proxy,
logstamp,
*kid_pid,basename(argv[0]));
else if (*kid_pid == 0)
be_proxied(from_proxy,to_proxy,
*from_server,*to_server,
argv);
else
die_fork();
teardown_and_exit(exitcode,kid_pid,rules,from_server,to_server);
}

void start_proxy(stralloc *greeting,filter_rule *rules,char **argv) {
@@ -339,10 +353,8 @@ void start_proxy(stralloc *greeting,filter_rule *rules,char **argv) {
int to_client = 1;
int kid_pid;

make_pipe(&from_proxy,&to_server);
make_pipe(&from_server,&to_proxy);

if ((kid_pid = unistd_fork()))
kid_pid = start_kid_with_pipes(&from_proxy,&to_server,&from_server,&to_proxy);
if (kid_pid)
be_proxy(from_client,to_client,
from_proxy,to_proxy,
from_server,to_server,
@@ -361,11 +373,11 @@ int read_and_process_until_either_end_closes(int from_client,int to_server,
stralloc *greeting,
filter_rule *rules,
stralloc *logstamp,
int *kid_pid,char **argv) {
int in_tls) {
char buf [SUBSTDIO_INSIZE];
int exitcode = EXIT_LATER_NORMALLY;
int tls_level = ucspitls_level(),
want_tls = 0, in_tls = 0,
want_tls = 0,
want_data = 0, in_data = 0;
stralloc client_requests = {0}, one_request = {0}, proxy_request = {0},
server_responses = {0}, one_response = {0}, proxy_response = {0};
@@ -399,9 +411,7 @@ int read_and_process_until_either_end_closes(int from_client,int to_server,
if (tls_level >= UCSPITLS_AVAILABLE && !in_tls) {
if (!tls_init() || !tls_info(die_nomem)) die_tls();
if (!env_put("FIXSMTPIOTLS=1")) die_nomem(__func__,"env_put");
stop_kid(*kid_pid,from_server,to_server);
start_kid(kid_pid,&from_server,&to_server,logstamp,argv);
in_tls = 1;
exitcode = BEGIN_STARTTLS_NOW;
}
}
}
@@ -6,6 +6,6 @@ int ends_data(stralloc *);
int is_last_line_of_response(stralloc *);
void parse_client_request(stralloc *,stralloc *,stralloc *);
int get_one_response(stralloc *,stralloc *);
int read_and_process_until_either_end_closes(int,int,int,int,stralloc *,filter_rule *,stralloc *,int *,char **);
int read_and_process_until_either_end_closes(int,int,int,int,stralloc *,filter_rule *,stralloc *,int);
void construct_proxy_request(stralloc *,filter_rule *,char *,stralloc *,stralloc *,int,int *,int,int *);
void start_proxy(stralloc *,filter_rule *,char **);

0 comments on commit 5e57ce4

Please sign in to comment.
You can’t perform that action at this time.