On Fri, Dec 18, 2020 at 05:50:27PM +0100, Theo Buehler wrote:
> On Fri, Dec 18, 2020 at 05:45:01PM +0100, Claudio Jeker wrote:
> > On Fri, Dec 18, 2020 at 01:46:49PM +0100, Theo Buehler wrote:
> > > On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> > > > On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > > > > rpki-client passes both empty strings and NULL strings as zero length
> > > > > objects. The unmarshal code then allocates memory in any case and so a
> > > > > NULL string is unmarshalled as empty string. This is not great,
> > > > > currently
> > > > > there are no empty strings but a fair amount of NULL strings.
> > > > > This diff changes the behaviour and now NULL is passed as NULL.
> > > > > This should simplify passing optional strings (e.g. in the entity
> > > > > queue
> > > > > code).
> > > >
> > > > I will commit this later today. It will help with some further cleanup
> > > > of
> > > > the codebase.
> > >
> > > I'm a bit torn about this. Some of the callers clearly do not expect
> > > having to deal with NULL.
> > >
> > > I see some *printf("%s", NULL) (for example in proc_rsync()) that should
> > > never happen but can now happen with this change. I'm unsure that there
> > > are no NULL derefs that this introduces. I'm fine with this going in if
> > > you intend to address this as part of the further cleanup work you
> > > envision.
> > >
> >
> > In most cases the code expects a non-empty string. For example in the
> > rsync.c case neither host nor mod are allowed to be NULL or "".
>
> Yes.
>
> > I guess adding an assert(host && mod) would be enough there.
>
> Right.
>
> > I actually prefer the code to blow up since as mentioned
> > empty strings are almost always wrong (e.g. empty filenames or hashes).
> > Indeed in all those cases a check for NULL should be added in the
> > unmarshal function.
>
> Go ahead. It certainly doesn't make things worse or (more) incorrect.
>
> ok tb
>
This is the next step. I added asserts for strings that must be set and
removed some of complications around optional strings. Especially cert.c
and some of the entityq code benefits from this.
--
:wq Claudio
Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.20
diff -u -p -r1.20 cert.c
--- cert.c 7 Dec 2020 13:23:01 -0000 1.20
+++ cert.c 18 Dec 2020 17:09:45 -0000
@@ -1262,7 +1262,6 @@ void
cert_buffer(char **b, size_t *bsz, size_t *bmax, const struct cert *p)
{
size_t i;
- int has_crl, has_aki;
io_simple_buffer(b, bsz, bmax, &p->valid, sizeof(int));
io_simple_buffer(b, bsz, bmax, &p->ipsz, sizeof(size_t));
@@ -1275,15 +1274,8 @@ cert_buffer(char **b, size_t *bsz, size_
io_str_buffer(b, bsz, bmax, p->mft);
io_str_buffer(b, bsz, bmax, p->notify);
-
- has_crl = (p->crl != NULL);
- io_simple_buffer(b, bsz, bmax, &has_crl, sizeof(int));
- if (has_crl)
- io_str_buffer(b, bsz, bmax, p->crl);
- has_aki = (p->aki != NULL);
- io_simple_buffer(b, bsz, bmax, &has_aki, sizeof(int));
- if (has_aki)
- io_str_buffer(b, bsz, bmax, p->aki);
+ io_str_buffer(b, bsz, bmax, p->crl);
+ io_str_buffer(b, bsz, bmax, p->aki);
io_str_buffer(b, bsz, bmax, p->ski);
}
@@ -1327,7 +1319,6 @@ cert_read(int fd)
{
struct cert *p;
size_t i;
- int has_crl, has_aki;
if ((p = calloc(1, sizeof(struct cert))) == NULL)
err(1, NULL);
@@ -1348,14 +1339,12 @@ cert_read(int fd)
cert_as_read(fd, &p->as[i]);
io_str_read(fd, &p->mft);
+ assert(p->mft);
io_str_read(fd, &p->notify);
- io_simple_read(fd, &has_crl, sizeof(int));
- if (has_crl)
- io_str_read(fd, &p->crl);
- io_simple_read(fd, &has_aki, sizeof(int));
- if (has_aki)
- io_str_read(fd, &p->aki);
+ io_str_read(fd, &p->crl);
+ io_str_read(fd, &p->aki);
io_str_read(fd, &p->ski);
+ assert(p->ski);
return p;
}
Index: main.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.86
diff -u -p -r1.86 main.c
--- main.c 9 Dec 2020 11:29:04 -0000 1.86
+++ main.c 18 Dec 2020 17:10:34 -0000
@@ -114,7 +114,6 @@ struct entity {
int has_pkey; /* whether pkey/sz is specified */
unsigned char *pkey; /* public key (optional) */
size_t pkeysz; /* public key length (optional) */
- int has_descr; /* whether descr is specified */
char *descr; /* tal description */
TAILQ_ENTRY(entity) entries;
};
@@ -233,9 +232,7 @@ entity_read_req(int fd, struct entity *e
io_simple_read(fd, &ent->has_pkey, sizeof(int));
if (ent->has_pkey)
io_buf_read_alloc(fd, (void **)&ent->pkey, &ent->pkeysz);
- io_simple_read(fd, &ent->has_descr, sizeof(int));
- if (ent->has_descr)
- io_str_read(fd, &ent->descr);
+ io_str_read(fd, &ent->descr);
}
/*
@@ -256,9 +253,7 @@ entity_buffer_req(char **b, size_t *bsz,
io_simple_buffer(b, bsz, bmax, &ent->has_pkey, sizeof(int));
if (ent->has_pkey)
io_buf_buffer(b, bsz, bmax, ent->pkey, ent->pkeysz);
- io_simple_buffer(b, bsz, bmax, &ent->has_descr, sizeof(int));
- if (ent->has_descr)
- io_str_buffer(b, bsz, bmax, ent->descr);
+ io_str_buffer(b, bsz, bmax, ent->descr);
}
/*
@@ -418,7 +413,6 @@ entityq_add(int fd, struct entityq *q, c
p->repo = (rp != NULL) ? (ssize_t)rp->id : -1;
p->has_dgst = dgst != NULL;
p->has_pkey = pkey != NULL;
- p->has_descr = descr != NULL;
if (p->has_dgst)
memcpy(p->dgst, dgst, sizeof(p->dgst));
if (p->has_pkey) {
@@ -427,7 +421,7 @@ entityq_add(int fd, struct entityq *q, c
err(1, "malloc");
memcpy(p->pkey, pkey, pkeysz);
}
- if (p->has_descr)
+ if (descr != NULL)
if ((p->descr = strdup(descr)) == NULL)
err(1, "strdup");
Index: mft.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.20
diff -u -p -r1.20 mft.c
--- mft.c 9 Dec 2020 11:25:08 -0000 1.20
+++ mft.c 18 Dec 2020 17:04:05 -0000
@@ -550,6 +550,7 @@ mft_read(int fd)
io_simple_read(fd, &p->stale, sizeof(int));
io_str_read(fd, &p->file);
+ assert(p->file);
io_simple_read(fd, &p->filesz, sizeof(size_t));
if ((p->files = calloc(p->filesz, sizeof(struct mftfile))) == NULL)
@@ -562,5 +563,7 @@ mft_read(int fd)
io_str_read(fd, &p->aki);
io_str_read(fd, &p->ski);
+ assert(p->aki && p->ski);
+
return p;
}
Index: roa.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
retrieving revision 1.9
diff -u -p -r1.9 roa.c
--- roa.c 12 Sep 2020 15:46:48 -0000 1.9
+++ roa.c 18 Dec 2020 17:03:09 -0000
@@ -445,6 +445,8 @@ roa_read(int fd)
io_str_read(fd, &p->aki);
io_str_read(fd, &p->ski);
io_str_read(fd, &p->tal);
+ assert(p->aki && p->ski && p->tal);
+
return p;
}
Index: rsync.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/rsync.c,v
retrieving revision 1.11
diff -u -p -r1.11 rsync.c
--- rsync.c 2 Dec 2020 15:31:15 -0000 1.11
+++ rsync.c 18 Dec 2020 16:59:45 -0000
@@ -296,6 +296,8 @@ proc_rsync(char *prog, char *bind_addr,
io_str_read(fd, &host);
io_str_read(fd, &mod);
+ assert(host);
+ assert(mod);
/*
* Create source and destination locations.
Index: tal.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/tal.c,v
retrieving revision 1.24
diff -u -p -r1.24 tal.c
--- tal.c 3 Dec 2020 15:02:12 -0000 1.24
+++ tal.c 18 Dec 2020 17:04:50 -0000
@@ -330,14 +330,17 @@ tal_read(int fd)
io_buf_read_alloc(fd, (void **)&p->pkey, &p->pkeysz);
assert(p->pkeysz > 0);
io_str_read(fd, &p->descr);
+ assert(p->descr);
io_simple_read(fd, &p->urisz, sizeof(size_t));
assert(p->urisz > 0);
if ((p->uri = calloc(p->urisz, sizeof(char *))) == NULL)
err(1, NULL);
- for (i = 0; i < p->urisz; i++)
+ for (i = 0; i < p->urisz; i++) {
io_str_read(fd, &p->uri[i]);
+ assert(p->uri[i]);
+ }
return p;
}
