On Mon, Nov 27, 2017 at 11:46:54PM +0100, Claudio Jeker wrote:
> On Mon, Nov 27, 2017 at 11:34:49PM +0100, Alexander Bluhm wrote:
> > On Mon, Nov 27, 2017 at 11:26:06PM +0100, Claudio Jeker wrote:
> > > Guess we should make the 3 fatalx() in that code different so that it
> > > becomes more clear on which call it fails.
> >
> > As in the diff below?
> >
> > > Wonder what kind of startup race we lose...
> >
> > I can reproduce one of the errors on my laptop:
> >
> > make run-regress-args-https-inspect.pl # create test certificates
> > find / >/dev/null # clear the buffer cache for a while
> > make run-regress-args-https-inspect.pl # fails at the first try
> >
> > ======== run-regress-args-https-inspect.pl ========
> > time SUDO=sudo KTRACE= RELAYD= perl -I/usr/src/regress/usr.sbin/relayd
> > /usr/src/regress/usr.sbin/relayd/relayd.pl copy
> > /usr/src/regress/usr.sbin/relayd/args-https-inspect.pl
> > Client child status: 65280 exit: 255 at
> > /usr/src/regress/usr.sbin/relayd/Proc.pm line 161.
> > *** Error 255 in /usr/src/regress/usr.sbin/relayd (Makefile:68
> > 'run-regress-args-https-inspect.pl')
> >
> > Now amd64, T430s, ssd, single machine
> >
> > execute: sudo relayd -dvv -f relayd.conf
> > startup
> > relay_load_certfiles: using ca certificate ca.crt
> > pfe: filter init done
> > socket_rlimit: max open files 1024
> > socket_rlimit: max open files 1024
> > relay_load_certfiles: using ca key ca.key
> > relay_load_certfiles: using certificate /etc/ssl/127.0.0.1.crt
> > relay_load_certfiles: using private key /etc/ssl/private/127.0.0.1.key
> > parent_tls_ticket_rekey: rekeying tickets
> > relay_privinit: adding relay relay-args-https-inspect.pl
> > protocol 1: name proto-args-https-inspect.pl
> > flags: used, relay flags: tls, tls client, tls inspect
> > tcp flags: no splice
> > tls flags: tlsv1.2, cipher-server-preference
> > tls session tickets: disabled
> > type: http
> > match request header log "foo" value "*"
> > match response header log "bar" value "*"
> > ca_engine_init: using RSA privsep engine
> > ca_engine_init: using RSA privsep engine
> > relay_tls_ctx_create: loading certificate
> > failed to load tls CA certificate: Invalid argument
> > relay: relay_launch: failed to create TLS context
> > pfe exiting, pid 18436
> > hce exiting, pid 69282
> > lost child: pid 5175 exited abnormally
> > ca exiting, pid 40825
> > parent terminating, pid 53770
> >
> > bluhm
> >
> > Index: ca.c
> > ===================================================================
> > RCS file: /mount/openbsd/cvs/src/usr.sbin/relayd/ca.c,v
> > retrieving revision 1.29
> > diff -u -p -r1.29 ca.c
> > --- ca.c 27 Nov 2017 21:06:25 -0000 1.29
> > +++ ca.c 27 Nov 2017 21:45:59 -0000
> > @@ -123,14 +123,14 @@ ca_launch(void)
> > if (rlay->rl_tls_cert_fd != -1) {
> > if ((buf = relay_load_fd(rlay->rl_tls_cert_fd,
> > &len)) == NULL)
> > - fatalx("ca_launch: cert");
> > + fatalx("ca_launch: cert relay_load_fd");
> >
> > if ((in = BIO_new_mem_buf(buf, len)) == NULL)
> > - fatalx("ca_launch: cert");
> > + fatalx("ca_launch: cert BIO_new_mem_buf");
> >
> > if ((cert = PEM_read_bio_X509(in, NULL,
> > NULL, NULL)) == NULL)
> > - fatalx("ca_launch: cert");
> > + fatalx("ca_launch: cert PEM_read_bio_X509");
> >
> > hash_x509(cert, hash, sizeof(hash));
> >
> > @@ -160,14 +160,14 @@ ca_launch(void)
> > if (rlay->rl_tls_cacert_fd != -1) {
> > if ((buf = relay_load_fd(rlay->rl_tls_cacert_fd,
> > &len)) == NULL)
> > - fatalx("ca_launch: cacert");
> > + fatalx("ca_launch: cacert relay_load_fd");
> >
> > if ((in = BIO_new_mem_buf(buf, len)) == NULL)
> > - fatalx("ca_launch: cacert");
> > + fatalx("ca_launch: cacert BIO_new_mem_buf");
> >
> > if ((cert = PEM_read_bio_X509(in, NULL,
> > NULL, NULL)) == NULL)
> > - fatalx("ca_launch: cacert");
> > + fatalx("ca_launch: cacert PEM_read_bio_X509");
> >
> > hash_x509(cert, hash, sizeof(hash));
> >
>
> Have the same diff in my tree. OK claudio@
> The problem is a race in relay_load_fd() according to guenther@ we should
> use pread instead of the lseek/ read combo. Lets see if that works.
>
Like this. Seems to fix it for me.
--
:wq Claudio
Index: relay.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.231
diff -u -p -r1.231 relay.c
--- relay.c 27 Nov 2017 21:09:55 -0000 1.231
+++ relay.c 27 Nov 2017 22:43:49 -0000
@@ -2580,15 +2580,14 @@ relay_load_fd(int fd, off_t *len)
struct stat st;
off_t size;
ssize_t rv;
+ int err;
if (fstat(fd, &st) != 0)
goto fail;
size = st.st_size;
if ((buf = calloc(1, size + 1)) == NULL)
goto fail;
- if (lseek(fd, 0, SEEK_SET) != 0)
- goto fail;
- if ((rv = read(fd, buf, size)) != size)
+ if ((rv = pread(fd, buf, size, 0)) != size)
goto fail;
close(fd);
@@ -2597,8 +2596,10 @@ relay_load_fd(int fd, off_t *len)
return (buf);
fail:
+ err = errno;
free(buf);
close(fd);
+ errno = err;
return (NULL);
}
