syslogd: less global, more malloc, more random

[Alexander Bluhm]

Hi,

Instead of having global variables containing the libevent structures,
allocate them with malloc.  This makes the address space layout
more random.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.184
diff -u -p -r1.184 syslogd.c
--- usr.sbin/syslogd/syslogd.c  9 Sep 2015 08:12:46 -   1.184
+++ usr.sbin/syslogd/syslogd.c  9 Sep 2015 10:06:30 -
@@ -275,9 +275,7 @@ int  linesize;
 
 int fd_ctlsock, fd_ctlconn, fd_klog, fd_sendsys,
 fd_udp, fd_udp6, fd_bind, fd_listen, fd_unix[MAXUNIX];
-struct eventev_ctlaccept, ev_ctlread, ev_ctlwrite, ev_klog, ev_sendsys,
-ev_udp, ev_udp6, ev_bind, ev_listen, ev_unix[MAXUNIX],
-ev_hup, ev_int, ev_quit, ev_term, ev_mark;
+struct event*ev_ctlaccept, *ev_ctlread, *ev_ctlwrite;
 
 LIST_HEAD(peer_list, peer) peers;
 struct peer {
@@ -344,6 +342,9 @@ int
 main(int argc, char *argv[])
 {
struct timeval   to;
+   struct event*ev_klog, *ev_sendsys,
+   *ev_udp, *ev_udp6, *ev_bind, *ev_listen, *ev_unix,
+   *ev_hup, *ev_int, *ev_quit, *ev_term, *ev_mark;
const char  *errstr;
char*p;
int  ch, i;
@@ -600,30 +601,47 @@ main(int argc, char *argv[])
/* Process is now unprivileged and inside a chroot */
event_init();
 
-   event_set(_ctlaccept, fd_ctlsock, EV_READ|EV_PERSIST,
-   ctlsock_acceptcb, _ctlaccept);
-   event_set(_ctlread, fd_ctlconn, EV_READ|EV_PERSIST,
-   ctlconn_readcb, _ctlread);
-   event_set(_ctlwrite, fd_ctlconn, EV_WRITE|EV_PERSIST,
-   ctlconn_writecb, _ctlwrite);
-   event_set(_klog, fd_klog, EV_READ|EV_PERSIST, klog_readcb, _klog);
-   event_set(_sendsys, fd_sendsys, EV_READ|EV_PERSIST, unix_readcb,
-   _sendsys);
-   event_set(_udp, fd_udp, EV_READ|EV_PERSIST, udp_readcb, _udp);
-   event_set(_udp6, fd_udp6, EV_READ|EV_PERSIST, udp_readcb, _udp6);
-   event_set(_bind, fd_bind, EV_READ|EV_PERSIST, udp_readcb, _bind);
-   event_set(_listen, fd_listen, EV_READ|EV_PERSIST, tcp_acceptcb,
-   _listen);
+   if ((ev_ctlaccept = malloc(sizeof(struct event))) == NULL ||
+   (ev_ctlread = malloc(sizeof(struct event))) == NULL ||
+   (ev_ctlwrite = malloc(sizeof(struct event))) == NULL ||
+   (ev_klog = malloc(sizeof(struct event))) == NULL ||
+   (ev_sendsys = malloc(sizeof(struct event))) == NULL ||
+   (ev_udp = malloc(sizeof(struct event))) == NULL ||
+   (ev_udp6 = malloc(sizeof(struct event))) == NULL ||
+   (ev_bind = malloc(sizeof(struct event))) == NULL ||
+   (ev_listen = malloc(sizeof(struct event))) == NULL ||
+   (ev_unix = reallocarray(NULL,nunix,sizeof(struct event))) == NULL ||
+   (ev_hup = malloc(sizeof(struct event))) == NULL ||
+   (ev_int = malloc(sizeof(struct event))) == NULL ||
+   (ev_quit = malloc(sizeof(struct event))) == NULL ||
+   (ev_term = malloc(sizeof(struct event))) == NULL ||
+   (ev_mark = malloc(sizeof(struct event))) == NULL)
+   err(1, "malloc");
+
+   event_set(ev_ctlaccept, fd_ctlsock, EV_READ|EV_PERSIST,
+   ctlsock_acceptcb, ev_ctlaccept);
+   event_set(ev_ctlread, fd_ctlconn, EV_READ|EV_PERSIST,
+   ctlconn_readcb, ev_ctlread);
+   event_set(ev_ctlwrite, fd_ctlconn, EV_WRITE|EV_PERSIST,
+   ctlconn_writecb, ev_ctlwrite);
+   event_set(ev_klog, fd_klog, EV_READ|EV_PERSIST, klog_readcb, ev_klog);
+   event_set(ev_sendsys, fd_sendsys, EV_READ|EV_PERSIST, unix_readcb,
+   ev_sendsys);
+   event_set(ev_udp, fd_udp, EV_READ|EV_PERSIST, udp_readcb, ev_udp);
+   event_set(ev_udp6, fd_udp6, EV_READ|EV_PERSIST, udp_readcb, ev_udp6);
+   event_set(ev_bind, fd_bind, EV_READ|EV_PERSIST, udp_readcb, ev_bind);
+   event_set(ev_listen, fd_listen, EV_READ|EV_PERSIST, tcp_acceptcb,
+   ev_listen);
for (i = 0; i < nunix; i++)
event_set(_unix[i], fd_unix[i], EV_READ|EV_PERSIST,
unix_readcb, _unix[i]);
 
-   signal_set(_hup, SIGHUP, init_signalcb, _hup);
-   signal_set(_int, SIGINT, die_signalcb, _int);
-   signal_set(_quit, SIGQUIT, die_signalcb, _quit);
-   signal_set(_term, SIGTERM, die_signalcb, _term);
+   signal_set(ev_hup, SIGHUP, init_signalcb, ev_hup);
+   signal_set(ev_int, SIGINT, die_signalcb, ev_int);
+   signal_set(ev_quit, SIGQUIT, die_signalcb, ev_quit);
+   signal_set(ev_term, SIGTERM, die_signalcb, ev_term);
 
-   evtimer_set(_mark, mark_timercb, _mark);
+   evtimer_set(ev_mark, mark_timercb, ev_mark);
 
init();
 
@@ -653,30 +671,30 @@ main(int argc, char *argv[])
priv_config_parse_done();
 
if 

Re: syslogd: less global, more malloc, more random

[Todd C. Miller]

On Wed, 09 Sep 2015 13:14:16 +0200, Alexander Bluhm wrote:

> Instead of having global variables containing the libevent structures,
> allocate them with malloc.  This makes the address space layout
> more random.

That huge if() makes things a bit harder to read.  Doing the NULL
check separately from the calls to malloc() might help.  E.g.

ev_ctlaccept = malloc(sizeof(struct event));
ev_ctlread = malloc(sizeof(struct event));
ev_ctlwrite = malloc(sizeof(struct event));
ev_klog = malloc(sizeof(struct event));
ev_sendsys = malloc(sizeof(struct event));
ev_udp = malloc(sizeof(struct event));
ev_udp6 = malloc(sizeof(struct event));
ev_bind = malloc(sizeof(struct event));
ev_listen = malloc(sizeof(struct event));
ev_unix = reallocarray(NULL, nunix, sizeof(struct event));
ev_hup = malloc(sizeof(struct event));
ev_int = malloc(sizeof(struct event));
ev_quit = malloc(sizeof(struct event));
ev_term = malloc(sizeof(struct event));
ev_mark = malloc(sizeof(struct event));

if (ev_ctlaccept == NULL || ev_ctlread == NULL || ev_ctlwrite == NULL ||
ev_klog == NULL || ev_sendsys == NULL || ev_udp == NULL ||
ev_udp6 == NULL || ev_bind == NULL || ev_int == NULL ||
ev_quit == NULL || ev_term == NULL || ev_mark == NULL)
err(1, "malloc");

 - todd

Re: syslogd: less global, more malloc, more random

[Todd C. Miller]

On Wed, 09 Sep 2015 18:57:05 +0200, Alexander Bluhm wrote:

> Hmm, then you have another list where you can forget one of them.
> ev_listen, ev_unix, ev_hup are missing in your example.

Yeah, that is the downside.  Looks like I deleted a little too much.

 - todd

Re: syslogd: less global, more malloc, more random

[Alexander Bluhm]

On Wed, Sep 09, 2015 at 08:55:12AM -0600, Todd C. Miller wrote:
> On Wed, 09 Sep 2015 13:14:16 +0200, Alexander Bluhm wrote:
> 
> > Instead of having global variables containing the libevent structures,
> > allocate them with malloc.  This makes the address space layout
> > more random.
> 
> That huge if() makes things a bit harder to read.  Doing the NULL
> check separately from the calls to malloc() might help.  E.g.

Hmm, then you have another list where you can forget one of them.
ev_listen, ev_unix, ev_hup are missing in your example.

bluhm

>   ev_ctlaccept = malloc(sizeof(struct event));
>   ev_ctlread = malloc(sizeof(struct event));
>   ev_ctlwrite = malloc(sizeof(struct event));
>   ev_klog = malloc(sizeof(struct event));
>   ev_sendsys = malloc(sizeof(struct event));
>   ev_udp = malloc(sizeof(struct event));
>   ev_udp6 = malloc(sizeof(struct event));
>   ev_bind = malloc(sizeof(struct event));
>   ev_listen = malloc(sizeof(struct event));
>   ev_unix = reallocarray(NULL, nunix, sizeof(struct event));
>   ev_hup = malloc(sizeof(struct event));
>   ev_int = malloc(sizeof(struct event));
>   ev_quit = malloc(sizeof(struct event));
>   ev_term = malloc(sizeof(struct event));
>   ev_mark = malloc(sizeof(struct event));
> 
>   if (ev_ctlaccept == NULL || ev_ctlread == NULL || ev_ctlwrite == NULL ||
>   ev_klog == NULL || ev_sendsys == NULL || ev_udp == NULL ||
>   ev_udp6 == NULL || ev_bind == NULL || ev_int == NULL ||
>   ev_quit == NULL || ev_term == NULL || ev_mark == NULL)
>   err(1, "malloc");
> 
>  - todd