Re: rpki-client: unchecked str(n)dup
On Thu, Mar 04, 2021 at 04:25:53PM +0100, Theo Buehler wrote:
> 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.c4 Mar 2021 14:24:54 - 1.4
> > > +++ http.c4 Mar 2021 14:46:20 -
> > > @@ -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.
I should probably add a comment there. On the other hand if that strdup()
failed I doubt the program will make all that more progress before blowing
up on some other malloc call.
Lets just be consistent and do the check there. OK claudio@
> >
> > > }
> > >
> > > 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.c4 Mar 2021 14:24:17 - 1.112
> > > +++ main.c4 Mar 2021 14:46:20 -
> > > @@ -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(, "%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 - 1.21
> > > +++ rsync.c 4 Mar 2021 14:46:20 -
> > > @@ -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.
Yeah, it is one of those where I'm constantly changing my mind about it.
As you noticed the NULL return does no harm. Maybe the comments need to be
slightly adjusted.
--
:wq Claudio
Re: rpki-client: unchecked str(n)dup
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 - 1.4
> > +++ http.c 4 Mar 2021 14:46:20 -
> > @@ -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 - 1.112
> > +++ main.c 4 Mar 2021 14:46:20 -
> > @@ -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(, "%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 - 1.21
> > +++ rsync.c 4 Mar 2021 14:46:20 -
> > @@ -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
Re: rpki-client: unchecked str(n)dup
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.c4 Mar 2021 14:24:54 - 1.4
> +++ http.c4 Mar 2021 14:46:20 -
> @@ -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.
> }
>
> 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.c4 Mar 2021 14:24:17 - 1.112
> +++ main.c4 Mar 2021 14:46:20 -
> @@ -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(, "%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@
> 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 - 1.21
> +++ rsync.c 4 Mar 2021 14:46:20 -
> @@ -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?
--
:wq Claudio
rpki-client: unchecked str(n)dup
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 - 1.4
+++ http.c 4 Mar 2021 14:46:20 -
@@ -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);
}
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 - 1.112
+++ main.c 4 Mar 2021 14:46:20 -
@@ -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(, "%s %s", stats.talnames, file) == -1)
err(1, NULL);
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 - 1.21
+++ rsync.c 4 Mar 2021 14:46:20 -
@@ -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
