Re: ntpd(8): use stack instead of heap
OK bcook@
On Fri, Dec 2, 2016 at 10:29 AM, Rafael Zalamena
wrote:
> On Sat, Oct 01, 2016 at 07:05:51PM +0200, Rafael Zalamena wrote:
> > The ntpd(8) constraint fork+exec diff changed the way the constraint
> > processes are created, but then it introduced new calloc()s to avoid
> > increasing diff size and to focus on the problem. Now that the fork+exec
> > is in, this diff make those variables to become a part of the stack.
> >
> > No functional changes, just changing variables storage location.
> >
> > ok?
>
> Ping.
>
> Updated diff to apply on the latest ntpd sources.
>
> ok?
>
> Index: usr.sbin/ntpd//constraint.c
> ===
> RCS file: /home/obsdcvs/src/usr.sbin/ntpd/constraint.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 constraint.c
> --- usr.sbin/ntpd//constraint.c 18 Oct 2016 22:05:47 - 1.34
> +++ usr.sbin/ntpd//constraint.c 2 Dec 2016 16:27:15 -
> @@ -321,8 +321,8 @@ priv_constraint_readquery(struct constra
> void
> priv_constraint_child(const char *pw_dir, uid_t pw_uid, gid_t pw_gid)
> {
> - struct constraint *cstr;
> - struct ntp_addr_msg *am;
> + struct constraintcstr;
> + struct ntp_addr_msg am;
> uint8_t *data;
> static char addr[NI_MAXHOST];
> struct timeval rectv, xmttv;
> @@ -336,10 +336,6 @@ priv_constraint_child(const char *pw_dir
> if (setpriority(PRIO_PROCESS, 0, 0) == -1)
> log_warn("could not set priority");
>
> - if ((cstr = calloc(1, sizeof(*cstr))) == NULL ||
> - (am = calloc(1, sizeof(*am))) == NULL)
> - fatal("%s: calloc", __func__);
> -
> /* Init TLS and load CA certs before chroot() */
> if (tls_init() == -1)
> fatalx("tls_init");
> @@ -368,9 +364,9 @@ priv_constraint_child(const char *pw_dir
> if (pledge("stdio inet", NULL) == -1)
> fatal("pledge");
>
> - cstr->fd = CONSTRAINT_PASSFD;
> - imsg_init(&cstr->ibuf, cstr->fd);
> - priv_constraint_readquery(cstr, am, &data);
> + cstr.fd = CONSTRAINT_PASSFD;
> + imsg_init(&cstr.ibuf, cstr.fd);
> + priv_constraint_readquery(&cstr, &am, &data);
>
> /*
> * Get the IP address as name and set the process title
> accordingly.
> @@ -378,8 +374,8 @@ priv_constraint_child(const char *pw_dir
> * any DNS operation, so it is safe to be called without the dns
> * pledge.
> */
> - if (getnameinfo((struct sockaddr *)&cstr->addr->ss,
> - SA_LEN((struct sockaddr *)&cstr->addr->ss),
> + if (getnameinfo((struct sockaddr *)&cstr.addr->ss,
> + SA_LEN((struct sockaddr *)&cstr.addr->ss),
> addr, sizeof(addr), NULL, 0,
> NI_NUMERICHOST) != 0)
> fatalx("%s getnameinfo", __func__);
> @@ -398,21 +394,21 @@ priv_constraint_child(const char *pw_dir
> fatal("%s fcntl F_SETFD", __func__);
>
> /* Get remaining data from imsg in the unpriv child */
> - if (am->namelen) {
> - if ((cstr->addr_head.name =
> - get_string(data, am->namelen)) == NULL)
> + if (am.namelen) {
> + if ((cstr.addr_head.name =
> + get_string(data, am.namelen)) == NULL)
> fatalx("invalid IMSG_CONSTRAINT_QUERY name");
> - data += am->namelen;
> + data += am.namelen;
> }
> - if (am->pathlen) {
> - if ((cstr->addr_head.path =
> - get_string(data, am->pathlen)) == NULL)
> + if (am.pathlen) {
> + if ((cstr.addr_head.path =
> + get_string(data, am.pathlen)) == NULL)
> fatalx("invalid IMSG_CONSTRAINT_QUERY path");
> }
>
> /* Run! */
> if ((ctx = httpsdate_query(addr,
> - CONSTRAINT_PORT, cstr->addr_head.name, cstr->addr_head.path,
> + CONSTRAINT_PORT, cstr.addr_head.name, cstr.addr_head.path,
> conf->ca, conf->ca_len, &rectv, &xmttv)) == NULL) {
> /* Abort with failure but without warning */
> exit(1);
> @@ -422,10 +418,10 @@ priv_constraint_child(const char *pw_dir
> iov[0].iov_len = sizeof(rectv);
> iov[1].iov_base = &xmttv;
> iov[1].iov_len = sizeof(xmttv);
> - imsg_composev(&cstr->ibuf,
> + imsg_composev(&cstr.ibuf,
> IMSG_CONSTRAINT_RESULT, 0, 0, -1, iov, 2);
> do {
> - rv = imsg_flush(&cstr->ibuf);
> + rv = imsg_flush(&cstr.ibuf);
> } while (rv == -1 && errno == EAGAIN);
>
> /* Tear down the TLS connection after sending the result */
>
>
Re: ntpd(8): use stack instead of heap
On Sat, Oct 01, 2016 at 07:05:51PM +0200, Rafael Zalamena wrote:
> The ntpd(8) constraint fork+exec diff changed the way the constraint
> processes are created, but then it introduced new calloc()s to avoid
> increasing diff size and to focus on the problem. Now that the fork+exec
> is in, this diff make those variables to become a part of the stack.
>
> No functional changes, just changing variables storage location.
>
> ok?
Ping.
Updated diff to apply on the latest ntpd sources.
ok?
Index: usr.sbin/ntpd//constraint.c
===
RCS file: /home/obsdcvs/src/usr.sbin/ntpd/constraint.c,v
retrieving revision 1.34
diff -u -p -r1.34 constraint.c
--- usr.sbin/ntpd//constraint.c 18 Oct 2016 22:05:47 - 1.34
+++ usr.sbin/ntpd//constraint.c 2 Dec 2016 16:27:15 -
@@ -321,8 +321,8 @@ priv_constraint_readquery(struct constra
void
priv_constraint_child(const char *pw_dir, uid_t pw_uid, gid_t pw_gid)
{
- struct constraint *cstr;
- struct ntp_addr_msg *am;
+ struct constraintcstr;
+ struct ntp_addr_msg am;
uint8_t *data;
static char addr[NI_MAXHOST];
struct timeval rectv, xmttv;
@@ -336,10 +336,6 @@ priv_constraint_child(const char *pw_dir
if (setpriority(PRIO_PROCESS, 0, 0) == -1)
log_warn("could not set priority");
- if ((cstr = calloc(1, sizeof(*cstr))) == NULL ||
- (am = calloc(1, sizeof(*am))) == NULL)
- fatal("%s: calloc", __func__);
-
/* Init TLS and load CA certs before chroot() */
if (tls_init() == -1)
fatalx("tls_init");
@@ -368,9 +364,9 @@ priv_constraint_child(const char *pw_dir
if (pledge("stdio inet", NULL) == -1)
fatal("pledge");
- cstr->fd = CONSTRAINT_PASSFD;
- imsg_init(&cstr->ibuf, cstr->fd);
- priv_constraint_readquery(cstr, am, &data);
+ cstr.fd = CONSTRAINT_PASSFD;
+ imsg_init(&cstr.ibuf, cstr.fd);
+ priv_constraint_readquery(&cstr, &am, &data);
/*
* Get the IP address as name and set the process title accordingly.
@@ -378,8 +374,8 @@ priv_constraint_child(const char *pw_dir
* any DNS operation, so it is safe to be called without the dns
* pledge.
*/
- if (getnameinfo((struct sockaddr *)&cstr->addr->ss,
- SA_LEN((struct sockaddr *)&cstr->addr->ss),
+ if (getnameinfo((struct sockaddr *)&cstr.addr->ss,
+ SA_LEN((struct sockaddr *)&cstr.addr->ss),
addr, sizeof(addr), NULL, 0,
NI_NUMERICHOST) != 0)
fatalx("%s getnameinfo", __func__);
@@ -398,21 +394,21 @@ priv_constraint_child(const char *pw_dir
fatal("%s fcntl F_SETFD", __func__);
/* Get remaining data from imsg in the unpriv child */
- if (am->namelen) {
- if ((cstr->addr_head.name =
- get_string(data, am->namelen)) == NULL)
+ if (am.namelen) {
+ if ((cstr.addr_head.name =
+ get_string(data, am.namelen)) == NULL)
fatalx("invalid IMSG_CONSTRAINT_QUERY name");
- data += am->namelen;
+ data += am.namelen;
}
- if (am->pathlen) {
- if ((cstr->addr_head.path =
- get_string(data, am->pathlen)) == NULL)
+ if (am.pathlen) {
+ if ((cstr.addr_head.path =
+ get_string(data, am.pathlen)) == NULL)
fatalx("invalid IMSG_CONSTRAINT_QUERY path");
}
/* Run! */
if ((ctx = httpsdate_query(addr,
- CONSTRAINT_PORT, cstr->addr_head.name, cstr->addr_head.path,
+ CONSTRAINT_PORT, cstr.addr_head.name, cstr.addr_head.path,
conf->ca, conf->ca_len, &rectv, &xmttv)) == NULL) {
/* Abort with failure but without warning */
exit(1);
@@ -422,10 +418,10 @@ priv_constraint_child(const char *pw_dir
iov[0].iov_len = sizeof(rectv);
iov[1].iov_base = &xmttv;
iov[1].iov_len = sizeof(xmttv);
- imsg_composev(&cstr->ibuf,
+ imsg_composev(&cstr.ibuf,
IMSG_CONSTRAINT_RESULT, 0, 0, -1, iov, 2);
do {
- rv = imsg_flush(&cstr->ibuf);
+ rv = imsg_flush(&cstr.ibuf);
} while (rv == -1 && errno == EAGAIN);
/* Tear down the TLS connection after sending the result */
ntpd(8): use stack instead of heap
The ntpd(8) constraint fork+exec diff changed the way the constraint
processes are created, but then it introduced new calloc()s to avoid
increasing diff size and to focus on the problem. Now that the fork+exec
is in, this diff make those variables to become a part of the stack.
No functional changes, just changing variables storage location.
ok?
Index: constraint.c
===
RCS file: /home/obsdcvs/src/usr.sbin/ntpd/constraint.c,v
retrieving revision 1.32
diff -u -p -r1.32 constraint.c
--- constraint.c26 Sep 2016 17:17:01 - 1.32
+++ constraint.c1 Oct 2016 18:54:35 -
@@ -317,8 +317,8 @@ priv_constraint_readquery(struct constra
void
priv_constraint_child(const char *pw_dir, uid_t pw_uid, gid_t pw_gid)
{
- struct constraint *cstr;
- struct ntp_addr_msg *am;
+ struct constraintcstr;
+ struct ntp_addr_msg am;
uint8_t *data;
static char addr[NI_MAXHOST];
struct timeval rectv, xmttv;
@@ -332,10 +332,6 @@ priv_constraint_child(const char *pw_dir
if (setpriority(PRIO_PROCESS, 0, 0) == -1)
log_warn("could not set priority");
- if ((cstr = calloc(1, sizeof(*cstr))) == NULL ||
- (am = calloc(1, sizeof(*am))) == NULL)
- fatal("%s: calloc", __func__);
-
/* Init TLS and load CA certs before chroot() */
if (tls_init() == -1)
fatalx("tls_init");
@@ -364,9 +360,9 @@ priv_constraint_child(const char *pw_dir
if (pledge("stdio inet", NULL) == -1)
fatal("pledge");
- cstr->fd = CONSTRAINT_PASSFD;
- imsg_init(&cstr->ibuf, cstr->fd);
- priv_constraint_readquery(cstr, am, &data);
+ cstr.fd = CONSTRAINT_PASSFD;
+ imsg_init(&cstr.ibuf, cstr.fd);
+ priv_constraint_readquery(&cstr, &am, &data);
/*
* Get the IP address as name and set the process title accordingly.
@@ -374,8 +370,8 @@ priv_constraint_child(const char *pw_dir
* any DNS operation, so it is safe to be called without the dns
* pledge.
*/
- if (getnameinfo((struct sockaddr *)&cstr->addr->ss,
- SA_LEN((struct sockaddr *)&cstr->addr->ss),
+ if (getnameinfo((struct sockaddr *)&cstr.addr->ss,
+ SA_LEN((struct sockaddr *)&cstr.addr->ss),
addr, sizeof(addr), NULL, 0,
NI_NUMERICHOST) != 0)
fatalx("%s getnameinfo", __func__);
@@ -394,21 +390,21 @@ priv_constraint_child(const char *pw_dir
fatal("%s fcntl F_SETFD", __func__);
/* Get remaining data from imsg in the unpriv child */
- if (am->namelen) {
- if ((cstr->addr_head.name =
- get_string(data, am->namelen)) == NULL)
+ if (am.namelen) {
+ if ((cstr.addr_head.name =
+ get_string(data, am.namelen)) == NULL)
fatalx("invalid IMSG_CONSTRAINT_QUERY name");
- data += am->namelen;
+ data += am.namelen;
}
- if (am->pathlen) {
- if ((cstr->addr_head.path =
- get_string(data, am->pathlen)) == NULL)
+ if (am.pathlen) {
+ if ((cstr.addr_head.path =
+ get_string(data, am.pathlen)) == NULL)
fatalx("invalid IMSG_CONSTRAINT_QUERY path");
}
/* Run! */
if ((ctx = httpsdate_query(addr,
- CONSTRAINT_PORT, cstr->addr_head.name, cstr->addr_head.path,
+ CONSTRAINT_PORT, cstr.addr_head.name, cstr.addr_head.path,
conf->ca, conf->ca_len, &rectv, &xmttv)) == NULL) {
/* Abort with failure but without warning */
exit(1);
@@ -418,9 +414,9 @@ priv_constraint_child(const char *pw_dir
iov[0].iov_len = sizeof(rectv);
iov[1].iov_base = &xmttv;
iov[1].iov_len = sizeof(xmttv);
- imsg_composev(&cstr->ibuf,
+ imsg_composev(&cstr.ibuf,
IMSG_CONSTRAINT_RESULT, 0, 0, -1, iov, 2);
- imsg_flush(&cstr->ibuf);
+ imsg_flush(&cstr.ibuf);
/* Tear down the TLS connection after sending the result */
httpsdate_free(ctx);
