Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[asan] various test and core bug repairs toward asan testing #1737

Merged
merged 6 commits into from Oct 16, 2018

Conversation

Projects
None yet
4 participants
@trws
Copy link
Member

trws commented Oct 15, 2018

Fixes #1722

The buf used in the first loop was probably sized before the largest of
the bad inputs was added. The comment over printable says give at least
double, it was giving 80 bytes for a 90 byte + input. Rather than
minimally increasing it, the 512 byte buffer used in the second loop is
now shared by both loops.

@trws trws changed the title test: use 512b buf for both loops test: fix stack overflow in test_kvs_eventlog Oct 15, 2018

@trws trws force-pushed the trws:kvs_eventlog_overflow branch from 7bd26c7 to 292a607 Oct 15, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Oct 15, 2018

Rebased. The test fail was due to the write error, so hopefully the updates on master will take care of it.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 15, 2018

Codecov Report

Merging #1737 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1737      +/-   ##
==========================================
- Coverage   79.33%   79.31%   -0.02%     
==========================================
  Files         184      184              
  Lines       34555    34559       +4     
==========================================
- Hits        27414    27411       -3     
- Misses       7141     7148       +7
Impacted Files Coverage Δ
src/common/libflux/mrpc.c 85.55% <100%> (+0.05%) ⬆️
src/common/libutil/read_all.c 86.2% <100%> (+0.49%) ⬆️
src/cmd/flux.c 84.04% <100%> (+0.17%) ⬆️
src/bindings/lua/lua-affinity/lua-affinity.c 97.81% <100%> (ø) ⬆️
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) ⬇️
src/common/libflux/handle.c 83.68% <0%> (-0.7%) ⬇️
src/cmd/flux-module.c 85.28% <0%> (-0.61%) ⬇️
src/common/libflux/message.c 80.95% <0%> (-0.24%) ⬇️
src/cmd/flux-event.c 77.97% <0%> (ø) ⬆️
src/modules/connector-local/local.c 73.16% <0%> (+0.19%) ⬆️
... and 1 more
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 15, 2018

The PR title is more descriptive of the commit than the commit title, so maybe use it to update the commit title?

Suggestion: if there are a few more asan/test fixes coming, perhaps we should hold off merging this and you can tack them on here?

@trws trws force-pushed the trws:kvs_eventlog_overflow branch from 292a607 to a7a6043 Oct 16, 2018

@trws trws changed the title test: fix stack overflow in test_kvs_eventlog [asan] various test and core bug repairs toward asan testing Oct 16, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Oct 16, 2018

With the various vixes from @chu11 and @SteVwonder plus what's in here, the entire test suite can run under asan and PASS. There are still many leaks, and certain tests tickle other issues, but running like this: LD_PRELOAD=<asan lib> ASAN_OPTIONS=handle_segv=0,detect_leaks=0 make check passes everything (well, all but valgrind, but combining both is just not a good idea).

trws added some commits Oct 15, 2018

test: fix stack overflow in test_kvs_eventlog
Fixes #1722

The buf used in the first loop was probably sized before the largest of
the bad inputs was added.  The comment over printable says give at least
double, it was giving 80 bytes for a 90 byte + input.  Rather than
minimally increasing it, the 512 byte buffer used in the second loop is
now shared by both loops.
lua/affinity: only read up to null
The lua_string_to_cpu_setp function always accessed the first two bytes,
even if there aren't two bytes to access.  Switched to strncmp to avoid
reading past the end of a string.

@trws trws force-pushed the trws:kvs_eventlog_overflow branch from a7a6043 to b565740 Oct 16, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 16, 2018

These are some good finds!

One nit: I'd prefer that we fix the subset of TAP tests that use the bad idiom, rather than create an ok() wrapper for all tests. The tests should print what's being tested or the expected result, not the obtained result. (diag() is the preferred way to print debugging info).

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 16, 2018

This should do it for that test

diff --git a/src/broker/test/attr.c b/src/broker/test/attr.c
index 8f91208..cb002aa 100644
--- a/src/broker/test/attr.c
+++ b/src/broker/test/attr.c
@@ -102,7 +102,7 @@ int main (int argc, char **argv)
      */
     val = attr_first (attrs);
     ok (val && !strcmp (val, "foo"),
-        "attr_first returned %s", val);
+        "attr_first returned foo");
     ok (attr_next (attrs) == NULL,
         "attr_next returned NULL");
     ok (attr_add (attrs, "foo1", "42", 0) == 0
@@ -112,19 +112,19 @@ int main (int argc, char **argv)
         "attr_add foo1, foo2, foo3, foo4 works");
     val = attr_first (attrs);
     ok (val && !strncmp (val, "foo", 3),
-        "attr_first returned %s", val);
+        "attr_first returned foo-prefixed attr");
     val = attr_next (attrs);
     ok (val && !strncmp (val, "foo", 3),
-        "attr_next returned %s", val);
+        "attr_next returned foo-prefixed attr");
     val = attr_next (attrs);
     ok (val && !strncmp (val, "foo", 3),
-        "attr_next returned %s", val);
+        "attr_next returned foo-prefixed attr");
     val = attr_next (attrs);
     ok (val && !strncmp (val, "foo", 3),
-        "attr_next returned %s", val);
+        "attr_next returned foo-prefixed attr");
     val = attr_next (attrs);
     ok (val && !strncmp (val, "foo", 3),
-        "attr_next returned %s", val);
+        "attr_next returned foo-prefixed attr");
     ok (attr_next (attrs) == NULL,
         "attr_next returned NULL");
 
@@ -134,28 +134,28 @@ int main (int argc, char **argv)
         "attr_add_active_int works");
     a = 0;
     ok (attr_get (attrs, "a", &val, NULL) == 0 && val && !strcmp (val, "0"),
-        "attr_get on active int tracks value: %s", val);
+        "attr_get on active int tracks val=0");
     a = 1;
     ok (attr_get (attrs, "a", &val, NULL) == 0 && !strcmp (val, "1"),
-        "attr_get on active int tracks value: %s", val);
+        "attr_get on active int tracks val=1");
     a = -1;
     ok (attr_get (attrs, "a", &val, NULL) == 0 && !strcmp (val, "-1"),
-        "attr_get on active int tracks value: %s", val);
+        "attr_get on active int tracks val=-1");
     a = INT_MAX - 1;
     ok (attr_get (attrs, "a", &val, NULL) == 0
         && strtol (val, NULL, 10) == INT_MAX - 1,
-        "attr_get on active int tracks value: %s", val);
+        "attr_get on active int tracks val=INT_MAX-1");
     a = INT_MIN + 1;
     ok (attr_get (attrs, "a", &val, NULL) == 0
         && strtol (val, NULL, 10) == INT_MIN + 1,
-        "attr_get on active int tracks value: %s", val);
+        "attr_get on active int tracks val=INT_MIN+1");
 
     ok (attr_set (attrs, "a", "0", false) == 0 && a == 0,
-        "attr_set on active int sets value: %d", a);
+        "attr_set on active int sets val=0");
     ok (attr_set (attrs, "a", "1", false) == 0 && a == 1,
-        "attr_set on active int sets value: %d", a);
+        "attr_set on active int sets val=1");
     ok (attr_set (attrs, "a", "-1", false) == 0 && a == -1,
-        "attr_set on active int sets value: %d", a);
+        "attr_set on active int sets val=-1");
     errno = 0;
     ok (attr_delete (attrs, "a", false) < 0 && errno == EPERM,
         "attr_delete on active attr fails with EPERM");
@@ -168,19 +168,19 @@ int main (int argc, char **argv)
         "attr_add_active_uint32 works");
     b = 0;
     ok (attr_get (attrs, "b", &val, NULL) == 0 && val && !strcmp (val, "0"),
-        "attr_get on active uin32_t tracks value: %s", val);
+        "attr_get on active uin32_t tracks val=0");
     b = 1;
     ok (attr_get (attrs, "b", &val, NULL) == 0 && !strcmp (val, "1"),
-        "attr_get on active uint32_t tracks value: %s", val);
+        "attr_get on active uint32_t tracks val=1");
     b = UINT_MAX - 1;
     ok (attr_get (attrs, "b", &val, NULL) == 0
         && strtoul (val, NULL, 10) == UINT_MAX - 1,
-        "attr_get on active uint32_t tracks value: %s", val);
+        "attr_get on active uint32_t tracks val=UINT_MAX-1");
 
     ok (attr_set (attrs, "b", "0", false) == 0 && b == 0,
-        "attr_set on active uint32_t sets value: %d", b);
+        "attr_set on active uint32_t sets val=0");
     ok (attr_set (attrs, "b", "1", false) == 0 && b == 1,
-        "attr_set on active uint32_t sets value: %d", b);
+        "attr_set on active uint32_t sets val=1");
     ok (attr_delete (attrs, "b", true) == 0,
         "attr_delete (force) works on active attr");
 
@@ -190,10 +190,10 @@ int main (int argc, char **argv)
         "attr_add_active_int FLUX_ATTRFLAG_IMMUTABLE works");
     c = 42;
     ok (attr_get (attrs, "c", &val, NULL) == 0 && val && !strcmp (val, "42"),
-        "attr_get returns initial value: %s", val);
+        "attr_get returns initial val=42");
     c = 43;
     ok (attr_get (attrs, "c", &val, NULL) == 0 && val && !strcmp (val, "42"),
-        "attr_get ignores value changes: %s", val);
+        "attr_get ignores value changes");
     errno = 0;
     ok (attr_delete (attrs, "c", true) < 0 && errno == EPERM,
         "attr_delete (force) fails with EPERM");
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 16, 2018

One nit: I'd prefer that we fix the subset of TAP tests that use the bad idiom, rather than create an ok() wrapper for all tests.

This is probably my fault as @trws had asked on slack which method to use and I had agreed that if we could fix it in ok() that would be nice (if accepted upstream). Sorry!

trws added some commits Oct 16, 2018

mrpc: destroy nodeset on success
While the failure path frees the nodeset, the success path did not.
util: null terminate the string in read_all
Found this through asan on sharness tests of dmesg, the flux-loggger
command uses read_all and then assumes that the result is
null-terminated.

@trws trws force-pushed the trws:kvs_eventlog_overflow branch from b565740 to d16f725 Oct 16, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Oct 16, 2018

Ok, applied the patch from @garlick (thanks!), and rebased out the original change to safe_ok. Assuming I didn't miss something in the rebase I think this is GTG.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 16, 2018

LGTM, thanks!

@garlick garlick merged commit 5680a0b into flux-framework:master Oct 16, 2018

3 checks passed

codecov/patch 100% of diff hit (target 79.33%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +20.66% compared to f252756
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.