On Thu, Mar 04, 2021 at 04:10:12PM +0100, Claudio Jeker wrote:
> On Thu, Mar 04, 2021 at 03:53:44PM +0100, Theo Buehler wrote:
> > The first two seem obvious oversights. The ones in rsync_base_uri()
> > would end up silently ignored:
> > queue_add_from_cert
> > repo_lookup
> > rsync_base_uri
> >
> > Index: http.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> > retrieving revision 1.4
> > diff -u -p -r1.4 http.c
> > --- http.c 4 Mar 2021 14:24:54 -0000 1.4
> > +++ http.c 4 Mar 2021 14:46:20 -0000
> > @@ -807,7 +807,8 @@ http_parse_header(struct http_connection
> > } else if (strncasecmp(cp, LAST_MODIFIED,
> > sizeof(LAST_MODIFIED) - 1) == 0) {
> > cp += sizeof(LAST_MODIFIED) - 1;
> > - conn->last_modified = strdup(cp);
> > + if ((conn->last_modified = strdup(cp)) == NULL)
> > + err(1, NULL);
>
> I did not make this a fatal error since the code works fine even if
> last_modified is NULL. This is just an extra data point to help pick up
> work on the next run (which is currently unused).
>
> I think both versions are correct. I don't mind if you commit this.
Alright, if it's deliberate, I'll leave it as it is.
>
> > }
> >
> > return 1;
> > Index: main.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > retrieving revision 1.112
> > diff -u -p -r1.112 main.c
> > --- main.c 4 Mar 2021 14:24:17 -0000 1.112
> > +++ main.c 4 Mar 2021 14:46:20 -0000
> > @@ -590,9 +590,10 @@ queue_add_tal(struct entityq *q, const c
> > buf = tal_read_file(file);
> >
> > /* Record tal for later reporting */
> > - if (stats.talnames == NULL)
> > - stats.talnames = strdup(file);
> > - else {
> > + if (stats.talnames == NULL) {
> > + if ((stats.talnames = strdup(file)) == NULL)
> > + err(1, NULL);
> > + } else {
> > char *tmp;
> > if (asprintf(&tmp, "%s %s", stats.talnames, file) == -1)
> > err(1, NULL);
>
> Hmm, I thought the asprintf call below was also unchecked since this is
> again optional data. On the other hand if the code already fails here to
> allocate a few bytes then I don't expect it to actually get much further.
>
> Since the else case now checks the asprintf return value I think it is
> fine to do the same for the strdup case. OK claudio@
I'll do that one for consistency.
>
> > Index: rsync.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/rsync.c,v
> > retrieving revision 1.21
> > diff -u -p -r1.21 rsync.c
> > --- rsync.c 4 Mar 2021 14:24:17 -0000 1.21
> > +++ rsync.c 4 Mar 2021 14:46:20 -0000
> > @@ -55,6 +55,7 @@ char *
> > rsync_base_uri(const char *uri)
> > {
> > const char *host, *module, *rest;
> > + char *base_uri;
> >
> > /* Case-insensitive rsync URI. */
> > if (strncasecmp(uri, "rsync://", 8) != 0) {
> > @@ -82,13 +83,18 @@ rsync_base_uri(const char *uri)
> >
> > /* The path component is optional. */
> > if ((rest = strchr(module, '/')) == NULL) {
> > - return strdup(uri);
> > + if ((base_uri = strdup(uri)) == NULL)
> > + err(1, NULL);
> > + return base_uri;
> > } else if (rest == module) {
> > warnx("%s: zero-length module", uri);
> > return NULL;
> > }
> >
> > - return strndup(uri, rest - uri);
> > + if ((base_uri = strndup(uri, rest - uri)) == NULL)
> > + err(1, NULL);
> > +
> > + return base_uri;
> > }
> >
> > static void
> >
>
> As you already noticed the NULL returns from strdup and strndup are
> handled by the caller. Do we really need this extra complexity?
One might argue the complexity is higher without the checks... Also,
the comment /* bad repository URI */ in queue_add_from_cert is inacurrate.
I don't insist, as there is no actual harm, it's just odd.
>
> --
> :wq Claudio
