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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 01 Nov 2019 03:39:30 +0000 Gerrit-HasComments: Yes
