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.
> }
>
> 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@
> 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?
--
:wq Claudio
