| Message ID | 6b4add9f-7947-9f81-48c9-83b77286d2e6@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [libnftnl,1/2] Avoid out of bound reads in tests. | expand |
Hi Maya, On Wed, Feb 17, 2021 at 10:45:45PM +0200, Maya Rashish wrote: > Our string isn't NUL-terminated. To avoid reading past > the last character, use strndup. Is this a theoretical problem or some static analisys tool is reporting out-of-bound memread? > Signed-off-by: Maya Rashish <mrashish@redhat.com> > --- > tests/nft-expr_match-test.c | 2 +- > tests/nft-expr_target-test.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/nft-expr_match-test.c b/tests/nft-expr_match-test.c > index 39a49d8..f6b7bc0 100644 > --- a/tests/nft-expr_match-test.c > +++ b/tests/nft-expr_match-test.c > @@ -71,7 +71,7 @@ int main(int argc, char *argv[]) > > nftnl_expr_set_str(ex, NFTNL_EXPR_MT_NAME, "Tests"); > nftnl_expr_set_u32(ex, NFTNL_EXPR_MT_REV, 0x12345678); > - nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strdup(data), sizeof(data)); > + nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strndup(data, sizeof(data)), sizeof(data)); > nftnl_rule_add_expr(a, ex); > > nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234); > diff --git a/tests/nft-expr_target-test.c b/tests/nft-expr_target-test.c > index ba56b27..a135b9c 100644 > --- a/tests/nft-expr_target-test.c > +++ b/tests/nft-expr_target-test.c > @@ -71,7 +71,7 @@ int main(int argc, char *argv[]) > > nftnl_expr_set(ex, NFTNL_EXPR_TG_NAME, "test", strlen("test")); > nftnl_expr_set_u32(ex, NFTNL_EXPR_TG_REV, 0x56781234); > - nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strdup(data), sizeof(data)); > + nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strndup(data, sizeof(data)), sizeof(data)); > nftnl_rule_add_expr(a, ex); > > nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234); > -- > 2.29.2 >
Hi Pablo, On 18/02/21 1:00 am, Pablo Neira Ayuso wrote: > Hi Maya, > > On Wed, Feb 17, 2021 at 10:45:45PM +0200, Maya Rashish wrote: >> Our string isn't NUL-terminated. To avoid reading past >> the last character, use strndup. > > Is this a theoretical problem or some static analisys tool is > reporting out-of-bound memread? As background, I had a difficult to diagnose stack corruption with a patched older version. I was hoping it'd just show up by running the tests with address sanitizer (I edited the Makefiles to add CFLAGS=-fsanitize=address and LDFLAGS=-lasan after configure) but it didn't. Address sanitizer usually reports actual problems, it runs the actual code with some elaborate memory map tricks that lets it detect violations. But might as well make the tests all run without complaints from address sanitizer while I am doing this.
diff --git a/tests/nft-expr_match-test.c b/tests/nft-expr_match-test.c index 39a49d8..f6b7bc0 100644 --- a/tests/nft-expr_match-test.c +++ b/tests/nft-expr_match-test.c @@ -71,7 +71,7 @@ int main(int argc, char *argv[]) nftnl_expr_set_str(ex, NFTNL_EXPR_MT_NAME, "Tests"); nftnl_expr_set_u32(ex, NFTNL_EXPR_MT_REV, 0x12345678); - nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strdup(data), sizeof(data)); + nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strndup(data, sizeof(data)), sizeof(data)); nftnl_rule_add_expr(a, ex); nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234); diff --git a/tests/nft-expr_target-test.c b/tests/nft-expr_target-test.c index ba56b27..a135b9c 100644 --- a/tests/nft-expr_target-test.c +++ b/tests/nft-expr_target-test.c @@ -71,7 +71,7 @@ int main(int argc, char *argv[]) nftnl_expr_set(ex, NFTNL_EXPR_TG_NAME, "test", strlen("test")); nftnl_expr_set_u32(ex, NFTNL_EXPR_TG_REV, 0x56781234); - nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strdup(data), sizeof(data)); + nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strndup(data, sizeof(data)), sizeof(data)); nftnl_rule_add_expr(a, ex); nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234);
Our string isn't NUL-terminated. To avoid reading past the last character, use strndup. Signed-off-by: Maya Rashish <mrashish@redhat.com> --- tests/nft-expr_match-test.c | 2 +- tests/nft-expr_target-test.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)