Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

fix substr crash #491

Merged
merged 4 commits into from
Apr 8, 2021
Merged

fix substr crash #491

merged 4 commits into from
Apr 8, 2021

Conversation

nevermore3
Copy link
Contributor

@nevermore3 nevermore3 added ready-for-review ready-for-testing PR: ready for the CI test labels Apr 8, 2021
@nevermore3 nevermore3 requested a review from a team April 8, 2021 07:19
@@ -1313,32 +1313,26 @@ FunctionManager::FunctionManager() {
attr.isPure_ = true;
attr.body_ = [](const auto &args) -> Value {
auto argSize = args.size();
if (argSize < 2 || argSize >3) {
if (argSize < 2 || argSize > 3) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The find method will check the count of arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

LOG(ERROR) << "Unexpected arguments count " << args.size();
return Value::kNullBadData;
}
if (args[0].isNull()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty is the same. but the type Signature has made sure the input args[0] must string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type Signature only use in deduce expr's type , When it encounters null, it returns directly in validator
So in the execution stage, we need to re-judge

user cannot enter EMPTY .so we not process EMPTY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the query input can be empty, such as pipe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EMPTY will do later ,all function have this problem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

TEST_FUNCTION(substr, std::vector<Value>({"hello", Value::kNullValue, 2}), Value::kNullBadType);
TEST_FUNCTION(substr, std::vector<Value>({"hello", 2, Value::kNullValue}), Value::kNullBadType);
TEST_FUNCTION(substr, std::vector<Value>({"hello", -1, 10}), Value::kNullBadData);
TEST_FUNCTION(substr, std::vector<Value>({"hello", 1, -2}), Value::kNullBadData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test when start large than string's size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alread exist in line 244

@yixinglu yixinglu merged commit c93c0e6 into vesoft-inc:master Apr 8, 2021
jude-zhu pushed a commit to jude-zhu/nebula-common that referenced this pull request Apr 9, 2021
* fix substr crash

* fix test error

Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com>
(cherry picked from commit c93c0e6)
bright-starry-sky pushed a commit that referenced this pull request Apr 12, 2021
* fix substr crash

* fix test error

Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com>
(cherry picked from commit c93c0e6)

Co-authored-by: jimingquan <mingquan.ji@vesoft.com>
@nevermore3 nevermore3 deleted the fix_substr_crash branch August 8, 2021 15:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants