You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2018/09/05 05:35:03 UTC
[kudu-CR] cfile-test: pass in IOContext on Open()
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11389
Change subject: cfile-test: pass in IOContext on Open()
......................................................................
cfile-test: pass in IOContext on Open()
Previously, on certain machines, cfile-test's TestDataCorruption would
fail 100% of the time when attempting to read from a null IOContext. The
passed-in ReaderOptions struct was constructed with the default
constructor, and because no default was set for its io_context member,
this resulted in undefined behavior, which explains why this wasn't
caught in pre-commit.
This patch updates cfile-test to correctly pass in an IOContext, and
assigns a default for ReaderOption's io_context.
Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_util.h
2 files changed, 5 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/11389/1
--
To view, visit http://gerrit.cloudera.org:8080/11389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Gerrit-Change-Number: 11389
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] cfile-test: pass in IOContext on Open()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11389 )
Change subject: cfile-test: pass in IOContext on Open()
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Gerrit-Change-Number: 11389
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 05 Sep 2018 05:38:37 +0000
Gerrit-HasComments: No
[kudu-CR] cfile-test: pass in IOContext on Open()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11389 )
Change subject: cfile-test: pass in IOContext on Open()
......................................................................
Patch Set 1:
Currently running other corruption-relevant tests on dist-test
--
To view, visit http://gerrit.cloudera.org:8080/11389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Gerrit-Change-Number: 11389
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 05 Sep 2018 05:37:22 +0000
Gerrit-HasComments: No
[kudu-CR] cfile-test: pass in IOContext on Open()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11389 )
Change subject: cfile-test: pass in IOContext on Open()
......................................................................
Patch Set 1: Code-Review+2
I'm curious why ASAN pre-commit build didn't catch that?
--
To view, visit http://gerrit.cloudera.org:8080/11389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Gerrit-Change-Number: 11389
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 05 Sep 2018 05:39:41 +0000
Gerrit-HasComments: No
[kudu-CR] cfile-test: pass in IOContext on Open()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11389 )
Change subject: cfile-test: pass in IOContext on Open()
......................................................................
Patch Set 2:
> > Patch Set 1: Code-Review+2
> >
> > I'm curious why ASAN pre-commit build didn't catch that?
>
> Yeah, it's surprising that it passed entirely on CentOs 6.6. That
> said, ASAN will cover:
> - Use after free (dangling pointer dereference)
> - Heap buffer overflow
> - Stack buffer overflow
> - Global buffer overflow
> - Use after return
> - Use after scope
> - Initialization order bugs
> - Memory leaks
>
> Of which this sort of error doesn't fall into any. It's certainly
> similar, but not quite any of them. UBSAN would have probably
> caught this though.
Well, I meant ASAN build, but our ASAN build has both address and undefined sanitizers included.
BTW, I'm not sure our ASAN configuration enables use-after-return and use-after-free traps.
--
To view, visit http://gerrit.cloudera.org:8080/11389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Gerrit-Change-Number: 11389
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 05 Sep 2018 06:19:30 +0000
Gerrit-HasComments: No
[kudu-CR] cfile-test: pass in IOContext on Open()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11389 )
Change subject: cfile-test: pass in IOContext on Open()
......................................................................
cfile-test: pass in IOContext on Open()
Previously, on certain machines, cfile-test's TestDataCorruption would
fail 100% of the time when attempting to read from a null IOContext. The
passed-in ReaderOptions struct was constructed with the default
constructor, and because no default was set for its io_context member,
this resulted in undefined behavior, which explains why this wasn't
caught in pre-commit.
This patch updates cfile-test to correctly pass in an IOContext, and
assigns a default for ReaderOption's io_context.
Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Reviewed-on: http://gerrit.cloudera.org:8080/11389
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_util.h
2 files changed, 5 insertions(+), 3 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Gerrit-Change-Number: 11389
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] cfile-test: pass in IOContext on Open()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11389 )
Change subject: cfile-test: pass in IOContext on Open()
......................................................................
Patch Set 1:
> Patch Set 1: Code-Review+2
>
> I'm curious why ASAN pre-commit build didn't catch that?
Yeah, it's surprising that it passed entirely on CentOs 6.6. That said, ASAN will cover:
- Use after free (dangling pointer dereference)
- Heap buffer overflow
- Stack buffer overflow
- Global buffer overflow
- Use after return
- Use after scope
- Initialization order bugs
- Memory leaks
Of which this sort of error doesn't fall into any. It's certainly similar, but not quite any of them. UBSAN would have probably caught this though.
--
To view, visit http://gerrit.cloudera.org:8080/11389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaed883d26075ed1823882a7f7b18834f39b0732d
Gerrit-Change-Number: 11389
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 05 Sep 2018 06:11:12 +0000
Gerrit-HasComments: No