From cc920da17c71c06fa0839cbc8ceb354d20d39c7b Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Mon, 28 Aug 2017 15:01:42 -0700 Subject: [PATCH 1/5] libflux/future: Add NULL future input checks In most of future library, check for corner case where user passes in NULL flux_future_t pointer. Fixes #1157 --- src/common/libflux/future.c | 63 ++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/common/libflux/future.c b/src/common/libflux/future.c index 66bfc342c1d1..5185721f6819 100644 --- a/src/common/libflux/future.c +++ b/src/common/libflux/future.c @@ -232,7 +232,8 @@ flux_future_t *flux_future_create (flux_reactor_t *r, */ void flux_future_set_flux (flux_future_t *f, flux_t *h) { - f->h = h; + if (f) + f->h = h; } /* Context dependent get of flux handle. @@ -243,7 +244,7 @@ flux_t *flux_future_get_flux (flux_future_t *f) { flux_t *h = NULL; - if (!f->h) { + if (!f || !f->h) { errno = EINVAL; goto done; } @@ -275,6 +276,10 @@ flux_t *flux_future_get_flux (flux_future_t *f) */ int flux_future_wait_for (flux_future_t *f, double timeout) { + if (!f) { + errno = EINVAL; + return -1; + } if (!f->result_valid && !f->result_errnum_valid) { if (timeout == 0.) { // don't setup 'now' context in this case errno = ETIMEDOUT; @@ -330,7 +335,7 @@ int flux_future_get (flux_future_t *f, void *result) int flux_future_then (flux_future_t *f, double timeout, flux_continuation_f cb, void *arg) { - if (!f->r || !cb || f->then != NULL) { + if (!f || !f->r || !cb || f->then != NULL) { errno = EINVAL; return -1; } @@ -353,6 +358,8 @@ int flux_future_then (flux_future_t *f, double timeout, */ void *flux_future_aux_get (flux_future_t *f, const char *name) { + if (!f) + return NULL; if (!f->aux) return NULL; return zhash_lookup (f->aux, name); @@ -368,7 +375,7 @@ int flux_future_aux_set (flux_future_t *f, const char *name, { char name_buf[32]; - if (!name && !destroy) { + if (!f || (!name && !destroy)) { errno = EINVAL; return -1; } @@ -393,33 +400,37 @@ int flux_future_aux_set (flux_future_t *f, const char *name, void flux_future_fulfill (flux_future_t *f, void *result, flux_free_f free_fn) { - if (f->result) { - if (f->result_free) - f->result_free (f->result); + if (f) { + if (f->result) { + if (f->result_free) + f->result_free (f->result); + } + f->result = result; + f->result_free = free_fn; + f->result_valid = true; + now_context_clear_timer (f->now); + then_context_clear_timer (f->then); + if (f->now) + flux_reactor_stop (f->now->r); + /* in "then" context, the main reactor prepare/check/idle watchers + * will run continuation in the next reactor loop for fairness. + */ } - f->result = result; - f->result_free = free_fn; - f->result_valid = true; - now_context_clear_timer (f->now); - then_context_clear_timer (f->then); - if (f->now) - flux_reactor_stop (f->now->r); - /* in "then" context, the main reactor prepare/check/idle watchers - * will run continuation in the next reactor loop for fairness. - */ } void flux_future_fulfill_error (flux_future_t *f, int errnum) { - f->result_errnum = errnum; - f->result_errnum_valid = true; - now_context_clear_timer (f->now); - then_context_clear_timer (f->then); - if (f->now) - flux_reactor_stop (f->now->r); - /* in "then" context, the main reactor prepare/check/idle watchers - * will run continuation in the next reactor loop for fairness. - */ + if (f) { + f->result_errnum = errnum; + f->result_errnum_valid = true; + now_context_clear_timer (f->now); + then_context_clear_timer (f->then); + if (f->now) + flux_reactor_stop (f->now->r); + /* in "then" context, the main reactor prepare/check/idle watchers + * will run continuation in the next reactor loop for fairness. + */ + } } /* timer - for flux_future_then() timeout From c75bfbaf2d967388b118f3c27708dae9d82ec69c Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Mon, 28 Aug 2017 15:11:26 -0700 Subject: [PATCH 2/5] libflux/future/test: Add new tests Add coverage for future == NULL corner cases tests. --- src/common/libflux/test/future.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/common/libflux/test/future.c b/src/common/libflux/test/future.c index 57bcd5f33b05..24d94f4858e9 100644 --- a/src/common/libflux/test/future.c +++ b/src/common/libflux/test/future.c @@ -55,11 +55,18 @@ void test_simple (void) ok (flux_future_aux_set (f, NULL, "bar", NULL) < 0 && errno == EINVAL, "flux_future_aux_set anon w/o destructor is EINVAL"); + errno = 0; + ok (flux_future_aux_set (NULL, "foo", "bar", aux_destroy) < 0 + && errno == EINVAL, + "flux_future_aux_set w/ NULL future is EINVAL"); aux_destroy_called = 0; aux_destroy_arg = NULL; ok (flux_future_aux_set (f, "foo", "bar", aux_destroy) == 0, "flux_future_aux_set works"); - char *p = flux_future_aux_get (f, "baz"); + char *p = flux_future_aux_get (NULL, "baz"); + ok (p == NULL, + "flux_future_aux_get with bad input returns NULL"); + p = flux_future_aux_get (f, "baz"); ok (p == NULL, "flux_future_aux_get of wrong value returns NULL"); p = flux_future_aux_get (f, "foo"); @@ -71,6 +78,9 @@ void test_simple (void) /* wait_for/get - no future_init; artificially call fulfill */ errno = 0; + ok (flux_future_wait_for (NULL, 0.) < 0 && errno == EINVAL, + "flux_future_wait_for w/ NULL future returns EINVAL"); + errno = 0; ok (flux_future_wait_for (f, 0.) < 0 && errno == ETIMEDOUT, "flux_future_wait_for initially times out"); errno = 0; @@ -87,6 +97,10 @@ void test_simple (void) "flux_future_get with NULL results argument also works"); /* continuation (result already ready) */ + errno = 0; + ok (flux_future_then (NULL, -1., contin, "nerp") < 0 + && errno == EINVAL, + "flux_future_then w/ NULL future returns EINVAL"); contin_called = 0; contin_arg = NULL; contin_get_rc = -42; From 72b8da7c188083e1fa5a371933ebcefea6404498 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Mon, 28 Aug 2017 15:18:09 -0700 Subject: [PATCH 3/5] libflux/future: Set errno in flux_future_aux_get Set errno appropriately when errors occur in flux_future_aux_get(). --- doc/man3/flux_future_create.adoc | 7 +++++-- src/common/libflux/future.c | 15 ++++++++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/doc/man3/flux_future_create.adoc b/doc/man3/flux_future_create.adoc index 49c033739aac..fc7c4e2a09ae 100644 --- a/doc/man3/flux_future_create.adoc +++ b/doc/man3/flux_future_create.adoc @@ -140,8 +140,8 @@ returned and errno is set appropriately. `flux_future_aux_set()` returns zero on success. On error, -1 is returned and errno is set appropriately. -`flux_future_aux_get()` returns the requested object on success. -If it is not found NULL. This function does not set errno. +`flux_future_aux_get()` returns the requested object on success. On +error, NULL is returned and errno is set appropriately. `flux_future_get_flux()` returns a flux_t handle on success. On error, NULL is returned and errno is set appropriately. @@ -156,6 +156,9 @@ Out of memory. EINVAL:: Invalid argument. +ENOENT:: +The requested object is not found. + AUTHOR ------ This page is maintained by the Flux community. diff --git a/src/common/libflux/future.c b/src/common/libflux/future.c index 5185721f6819..8ade58469d7e 100644 --- a/src/common/libflux/future.c +++ b/src/common/libflux/future.c @@ -358,11 +358,20 @@ int flux_future_then (flux_future_t *f, double timeout, */ void *flux_future_aux_get (flux_future_t *f, const char *name) { - if (!f) + void *rv; + + if (!f) { + errno = EINVAL; return NULL; - if (!f->aux) + } + if (!f->aux) { + errno = ENOENT; return NULL; - return zhash_lookup (f->aux, name); + } + /* zhash_lookup won't set errno if not found */ + if (!(rv = zhash_lookup (f->aux, name))) + errno = ENOENT; + return rv; } /* Store 'aux' object by name. Allow "anonymous" (name=NULL) objects to From e0f06324f4307a184ddfa3978177f5065df440c7 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Mon, 28 Aug 2017 15:20:36 -0700 Subject: [PATCH 4/5] libflux/future/test: Add flux_future_aux_get tests Add tests to check for errno return values from flux_future_aux_get(). --- src/common/libflux/test/future.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/common/libflux/test/future.c b/src/common/libflux/test/future.c index 24d94f4858e9..8c5202eba72f 100644 --- a/src/common/libflux/test/future.c +++ b/src/common/libflux/test/future.c @@ -50,6 +50,13 @@ void test_simple (void) if (!f) BAIL_OUT ("flux_future_create failed"); + /* before aux is set */ + errno = 0; + char *p = flux_future_aux_get (f, "foo"); + ok (p == NULL + && errno == ENOENT, + "flux_future_aux_get of wrong value returns ENOENT"); + /* aux */ errno = 0; ok (flux_future_aux_set (f, NULL, "bar", NULL) < 0 @@ -63,12 +70,16 @@ void test_simple (void) aux_destroy_arg = NULL; ok (flux_future_aux_set (f, "foo", "bar", aux_destroy) == 0, "flux_future_aux_set works"); - char *p = flux_future_aux_get (NULL, "baz"); - ok (p == NULL, - "flux_future_aux_get with bad input returns NULL"); + errno = 0; + p = flux_future_aux_get (NULL, "baz"); + ok (p == NULL + && errno == EINVAL, + "flux_future_aux_get with bad input returns EINVAL"); + errno = 0; p = flux_future_aux_get (f, "baz"); - ok (p == NULL, - "flux_future_aux_get of wrong value returns NULL"); + ok (p == NULL + && errno == ENOENT, + "flux_future_aux_get of wrong value returns ENOENT"); p = flux_future_aux_get (f, "foo"); ok (p != NULL && !strcmp (p, "bar"), "flux_future_aux_get of known returns it"); From 87b5524ef94f9d6c8f80bd42a26e6da2b91d9042 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Mon, 28 Aug 2017 15:22:51 -0700 Subject: [PATCH 5/5] libkvs/test: Add kvs_lookup coverage --- src/common/libkvs/test/kvs_lookup.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/common/libkvs/test/kvs_lookup.c b/src/common/libkvs/test/kvs_lookup.c index 45a35719ad0e..fc66966278ab 100644 --- a/src/common/libkvs/test/kvs_lookup.c +++ b/src/common/libkvs/test/kvs_lookup.c @@ -20,6 +20,14 @@ void errors (void) errno = 0; ok (flux_kvs_lookupat (NULL, 0, NULL, NULL) == NULL && errno == EINVAL, "flux_kvs_lookupat fails on bad input"); + + errno = 0; + ok (flux_kvs_lookup_get (NULL, NULL) < 0 && errno == EINVAL, + "flux_kvs_lookup_get fails on bad input"); + + errno = 0; + ok (flux_kvs_lookup_get_unpack (NULL, NULL) < 0 && errno == EINVAL, + "flux_kvs_lookup_get_unpack fails on bad input"); } int main (int argc, char *argv[])