Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14291 )
Change subject: IMPALA-8704: ISO:SQL:2016 datetime patterns - Milestone 2 ...................................................................... Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-parser.cc File be/src/runtime/datetime-iso-sql-format-parser.cc: http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-parser.cc@214 PS11, Line 214: DCHECK(tok != nullptr); DCHECK(tok->val <= *format < tok->val + tok->len); http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-parser.cc@217 PS11, Line 217: strncmp Is it possible that strncmp (here or in the next line) overruns [tok->val, tok->val + tok->len) char-range? http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-parser.cc@255 PS11, Line 255: while (dt_ctx.toks[*current_tok_idx].type == TEXT && Add *current_tok_idx < dt_ctx.toks.size() condition to the while condition otherwise we might overrun the dt_ctx.toks buffer. http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-tokenizer.cc File be/src/runtime/datetime-iso-sql-format-tokenizer.cc: http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@84 PS11, Line 84: const char* str_end = dt_ctx_->fmt + dt_ctx_->fmt_len; As explained in L271, 'str_end' is not needed. You can also add DCHECK(dt_ctx_->fmt[dt_ctx_->fmt_len] == '\0') here. http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@111 PS11, Line 111: const char* str_end = dt_ctx_->fmt + dt_ctx_->fmt_len; As explained in L271, 'str_end' is not needed. http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@271 PS11, Line 271: while (current_pos < str_end) { If we assume that 'str_start' points to a null-terminated string, we don't really need to use 'str_end' here and elsewhere. You can just do: while (*current_pos != '\0') {..} http://gerrit.cloudera.org:8080/#/c/14291/11/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/14291/11/tests/query_test/test_cast_with_format.py@1101 PS11, Line 1101: FXFMFXYYYY-MM-DD 'FXFXYYYY-MM-DD' should fail too right? If yes, please add a test for that format string too. -- To view, visit http://gerrit.cloudera.org:8080/14291 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30d2f6656054371476aaa8bd0d51f572b9369855 Gerrit-Change-Number: 14291 Gerrit-PatchSet: 11 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 29 Oct 2019 13:58:53 +0000 Gerrit-HasComments: Yes
