You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/10/30 04:55:16 UTC
[kudu-CR] gutil/strings: fix UB in FindNth
Hello Alexey Serbin, Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14585
to review the following change.
Change subject: gutil/strings: fix UB in FindNth
......................................................................
gutil/strings: fix UB in FindNth
It's not OK to overflow an unsigned type (size_t) in this way. Newer
versions of the code (in other Google codebases) address the problem by
using an int instead of size_t, so we'll do the same thing too.
src/kudu/gutil/strings/util.cc:1030:34: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'
#0 0x7fcee8a0798e in FindNth(StringPiece, char, int) /src/kudu/gutil/strings/util.cc:1030:34
Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
---
M src/kudu/gutil/strings/string_util-test.cc
M src/kudu/gutil/strings/util.cc
2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/14585/1
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] gutil/strings: fix UBSAN error in FindNth
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14585
to look at the new patch set (#3).
Change subject: gutil/strings: fix UBSAN error in FindNth
......................................................................
gutil/strings: fix UBSAN error in FindNth
UBSAN complains when we overflow an unsigned type (size_t). It's not
undefined behavior, but UBSAN's position is that these kinds of overflows
are often unintentional and signal a bug.
Newer versions of the code (in other Google codebases) address the problem
by using an int instead of size_t, so we'll do the same thing too.
src/kudu/gutil/strings/util.cc:1030:34: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'
#0 0x7fcee8a0798e in FindNth(StringPiece, char, int) /src/kudu/gutil/strings/util.cc:1030:34
Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
---
M src/kudu/gutil/strings/string_util-test.cc
M src/kudu/gutil/strings/util.cc
2 files changed, 11 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/14585/3
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@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 (120)
[kudu-CR] gutil/strings: fix UBSAN error in FindNth
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/14585 )
Change subject: gutil/strings: fix UBSAN error in FindNth
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] gutil/strings: fix UB in FindNth
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14585
to look at the new patch set (#2).
Change subject: gutil/strings: fix UB in FindNth
......................................................................
gutil/strings: fix UB in FindNth
It's not OK to overflow an unsigned type (size_t) in this way. Newer
versions of the code (in other Google codebases) address the problem by
using an int instead of size_t, so we'll do the same thing too.
src/kudu/gutil/strings/util.cc:1030:34: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'
#0 0x7fcee8a0798e in FindNth(StringPiece, char, int) /src/kudu/gutil/strings/util.cc:1030:34
Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
---
M src/kudu/gutil/strings/string_util-test.cc
M src/kudu/gutil/strings/util.cc
2 files changed, 11 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/14585/2
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] gutil/strings: fix UBSAN error in FindNth
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14585 )
Change subject: gutil/strings: fix UBSAN error in FindNth
......................................................................
gutil/strings: fix UBSAN error in FindNth
UBSAN complains when we overflow an unsigned type (size_t). It's not
undefined behavior, but UBSAN's position is that these kinds of overflows
are often unintentional and signal a bug.
Newer versions of the code (in other Google codebases) address the problem
by using an int instead of size_t, so we'll do the same thing too.
src/kudu/gutil/strings/util.cc:1030:34: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'
#0 0x7fcee8a0798e in FindNth(StringPiece, char, int) /src/kudu/gutil/strings/util.cc:1030:34
Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Reviewed-on: http://gerrit.cloudera.org:8080/14585
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/gutil/strings/string_util-test.cc
M src/kudu/gutil/strings/util.cc
2 files changed, 11 insertions(+), 1 deletion(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Adar Dembo: Verified
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] gutil/strings: fix UB in FindNth
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14585 )
Change subject: gutil/strings: fix UB in FindNth
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG@9
PS1, Line 9: It's not OK to overflow an unsigned type (size_t) in this way
> Why? As of my knowledge, size_t is unsigned type. It overflows safely, an
I pasted the UBSAN error below. The docs say:
-fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where the result of an unsigned integer computation cannot be represented in its type. Unlike signed integer overflow, this is not undefined behavior, but it is often unintentional.
I can add this to the build-support/ubsan-blacklist.txt instead, but we've only done that for thirdparty libraries.
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG@11
PS1, Line 11: int instead of size_t
> I think this is even more unsafe: it's not safe to overflow signed types.
We're initializing a variable to -1 and then adding 1 to it. That overflows an unsigned type (intentionally), but not a signed type.
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/string_util-test.cc
File src/kudu/gutil/strings/string_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/string_util-test.cc@64
PS1, Line 64: }
> Could you add a test for:
Done
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc
File src/kudu/gutil/strings/util.cc:
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc@1026
PS1, Line 1026: int n
> What if I pass std::max<int>() here?
Shouldn't matter; n is only used to derive i, and i is only used to control the number of loop iterations. Neither i nor n are involved in the find_first_of() call.
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc@1030
PS1, Line 1030: s.find_first_of(c, pos + 1);
> Instead, could we leave 'pos' as 'size_t' and we rewrite it like:
I thought about doing that, but it adds an extra branch to each iteration of the loop. That's why I opted to change the type to int instead.
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@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 (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 00:21:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil/strings: fix UB in FindNth
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14585 )
Change subject: gutil/strings: fix UB in FindNth
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG@9
PS1, Line 9: It's not OK to overflow an unsigned type (size_t) in this way
Why? As of my knowledge, size_t is unsigned type. It overflows safely, and that's the documented behavior.
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG@11
PS1, Line 11: int instead of size_t
I think this is even more unsafe: it's not safe to overflow signed types.
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/string_util-test.cc
File src/kudu/gutil/strings/string_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/string_util-test.cc@64
PS1, Line 64: }
Could you add a test for:
FindNth("abcd", 'e', std::max<int>()) ?
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc
File src/kudu/gutil/strings/util.cc:
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc@1026
PS1, Line 1026: int n
What if I pass std::max<int>() here?
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc@1030
PS1, Line 1030: s.find_first_of(c, pos + 1);
Instead, could we leave 'pos' as 'size_t' and we rewrite it like:
pos = s.find_first_of(c, (pos == string::npos) ? 0 : pos + 1);
?
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Oct 2019 18:36:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil/strings: fix UBSAN error in FindNth
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14585 )
Change subject: gutil/strings: fix UBSAN error in FindNth
......................................................................
Patch Set 3: Verified+1
Overriding Jenkins, NTP failure with built-in client.
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@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 (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 04:04:30 +0000
Gerrit-HasComments: No
[kudu-CR] gutil/strings: fix UBSAN error in FindNth
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14585 )
Change subject: gutil/strings: fix UBSAN error in FindNth
......................................................................
Patch Set 3: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG@9
PS1, Line 9: UBSAN complains when we overflow an unsigned type (size_t). I
> I pasted the UBSAN error below. The docs say:
Thank you for the clarification. So, UBSAN tried to help with unintentional overflow here, I see.
http://gerrit.cloudera.org:8080/#/c/14585/1//COMMIT_MSG@11
PS1, Line 11: unintentional and si
> We're initializing a variable to -1 and then adding 1 to it. That overflows
I was concerned about something like FindNth(string(std::numeric_limits<int>::max() + 1, 'x'), 'x', std::numeric_limits<int>::max())
But I think that regardless of outcome, this is an artificial case anyways for the use cases this method is targeted.
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc
File src/kudu/gutil/strings/util.cc:
http://gerrit.cloudera.org:8080/#/c/14585/1/src/kudu/gutil/strings/util.cc@1030
PS1, Line 1030: s.find_first_of(c, pos + 1);
> I thought about doing that, but it adds an extra branch to each iteration o
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@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 (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 03:39:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil/strings: fix UB in FindNth
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14585 )
Change subject: gutil/strings: fix UB in FindNth
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00a36f999a8c3b13469600b785da95c487643d4a
Gerrit-Change-Number: 14585
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Oct 2019 17:54:13 +0000
Gerrit-HasComments: No