Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix address sanitize reported stack-use-after-scope #1721

Merged
merged 3 commits into from
Jul 21, 2023
Merged

fix address sanitize reported stack-use-after-scope #1721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f13aef9
fix address sanitize
tedli Jul 13, 2023
396299d
Merge remote-tracking branch 'upstream/unstable' into asan
tedli Jul 21, 2023
bfd07fd
run unit test in build_macos ci task
tedli Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/pika.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,11 @@ jobs:
cmake -B ${{github.workspace}}/build -S . -DCMAKE_C_COMPILER=/usr/local/opt/gcc@10/bin/gcc-10 -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}}
cmake --build ${{github.workspace}}/build --config ${{env.BUILD_TYPE}}

# - name: Test
# working-directory: ${{github.workspace}}/build
# run: |
# export CC=/usr/local/opt/gcc@10/bin/gcc-10
# ctest -C ${{env.BUILD_TYPE}}
- name: Test
working-directory: ${{github.workspace}}/build
run: |
export CC=/usr/local/opt/gcc@10/bin/gcc-10
ctest -C ${{env.BUILD_TYPE}}

- name: Unit Test
working-directory: ${{github.workspace}}
Copy link

Choose a reason for hiding this comment

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

Based on the code patch you provided, here's a brief code review:

  1. The code changes seem to modify a CI/CD configuration file.
  2. In the jobs section, there is a change related to the Test step. Previously, it was commented out, but now it has been uncommented and its content modified.
  3. The modified Test step sets the working-directory to $github.workspace/build and runs the following commands in that directory:
    • Sets the environment variable CC to /usr/local/opt/gcc@10/bin/gcc-10.
    • Executes ctest -C ${env.BUILD_TYPE}.

Review suggestions:

  • As the changes primarily focus on un-commenting and modifying the Test step, it appears there are no obvious bug risks based on the given context.
  • To further enhance the code, you could consider adding error handling and outputs for better visibility into the test results, for example, by including options like --output-on-failure or capturing the test output logs.
  • It might be beneficial to provide more details or context about the purpose of these changes or any specific issues you encountered while testing this configuration.

Please note that without additional details or the complete context, it may not be possible to identify potential issues or improvements accurately.

Expand Down
4 changes: 2 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ execute_process(COMMAND uname -p OUTPUT_VARIABLE HOST_ARCH)
string(TOLOWER ${HOST_ARCH} HOST_ARCH)

if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE RElEASE)
set(CMAKE_BUILD_TYPE RELEASE)
endif()

string(TOUPPER ${CMAKE_BUILD_TYPE} BUILD_TYPE)
Expand All @@ -50,7 +50,7 @@ elseif(${BUILD_TYPE} STREQUAL MINSIZEREL)
elseif(${BUILD_TYPE} STREQUAL RELWITHDEBINFO)
set(LIB_BUILD_TYPE RELWITHDEBINFO)
else()
set(LIB_BUILD_TYPE RElEASE)
set(LIB_BUILD_TYPE RELEASE)
set(CMAKE_CXX_FLAGS_RELEASE "-O2 -DNDEBUG")
endif()

Copy link

Choose a reason for hiding this comment

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

The code patch you provided appears to be correcting a typo in setting the CMAKE_BUILD_TYPE and LIB_BUILD_TYPE variables. Here are my observations and suggestions:

  1. Line 34: The COMMAND uname -p OUTPUT_VARIABLE HOST_ARCH might need to be enclosed in double quotes to ensure proper execution.

  2. Lines 35-37: The string(TOLOWER ${HOST_ARCH} HOST_ARCH) converts the value of ${HOST_ARCH} to lowercase. Make sure this is the desired behavior.

  3. Lines 40-42: The condition checks if CMAKE_BUILD_TYPE is empty (NOT CMAKE_BUILD_TYPE). If it's empty, it sets CMAKE_BUILD_TYPE to "RELEASE". This ensures that CMAKE_BUILD_TYPE is always set to a valid value.

  4. Lines 47-48: Similar to the previous point, these lines handle the case where ${BUILD_TYPE} is not recognized. It will set LIB_BUILD_TYPE to "RELEASE" and update CMAKE_CXX_FLAGS_RELEASE with the desired optimization flags.

Overall, the code patch seems to address potential issues and improve consistency by ensuring that the build types are consistently set to "RELEASE" when not explicitly defined. However, without a complete understanding of the context and the rest of the code, it is difficult to provide a comprehensive review. It would be helpful to provide more details about the purpose and context of this code for a more accurate assessment.

Copy link

Choose a reason for hiding this comment

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

In the provided code patch, there are a few changes made to the CMakeLists.txt file. Here's a brief code review:

  1. Line 34: The code uses the execute_process function to execute the uname -p command and stores the output in the variable HOST_ARCH.

  2. Line 35: It converts the value of HOST_ARCH to lowercase using string(TOLOWER).

  3. Lines 37-40: This block handles the case when the CMAKE_BUILD_TYPE variable is not set. It sets the CMAKE_BUILD_TYPE to "RELEASE" if it is not already specified.

  4. Line 43: It converts the value of CMAKE_BUILD_TYPE to uppercase using string(TOUPPER) and stores it in the variable BUILD_TYPE.

  5. Lines 46-49: These conditions check the value of BUILD_TYPE and set the LIB_BUILD_TYPE accordingly. If it is neither "DEBUG" nor "RELEASE," it sets it to "RELEASE" and also sets the CMAKE_CXX_FLAGS_RELEASE to "-O2 -DNDEBUG".

Overall, the code changes seem to fix spelling errors related to the RELEASE build type. The code review does not indicate any bug risks. However, since this is a partial code segment, it is difficult to comment on the broader context and functionality.

Expand Down
51 changes: 24 additions & 27 deletions src/storage/src/redis_hashes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,8 @@ Status RedisHashes::HIncrby(const Slice& key, const Slice& field, int64_t value,
std::string meta_value;

Status s = db_->Get(default_read_options_, handles_[0], key, &meta_value);
char value_buf[32] = {0};
char meta_value_buf[4] = {0};
if (s.ok()) {
ParsedHashesMetaValue parsed_hashes_meta_value(&meta_value);
if (parsed_hashes_meta_value.IsStale() || parsed_hashes_meta_value.count() == 0) {
Expand All @@ -325,9 +327,8 @@ Status RedisHashes::HIncrby(const Slice& key, const Slice& field, int64_t value,
parsed_hashes_meta_value.set_timestamp(0);
batch.Put(handles_[0], key, meta_value);
HashesDataKey hashes_data_key(key, version, field);
char buf[32];
Int64ToStr(buf, 32, value);
batch.Put(handles_[1], hashes_data_key.Encode(), buf);
Int64ToStr(value_buf, 32, value);
batch.Put(handles_[1], hashes_data_key.Encode(), value_buf);
*ret = value;
} else {
version = parsed_hashes_meta_value.version();
Expand All @@ -342,32 +343,28 @@ Status RedisHashes::HIncrby(const Slice& key, const Slice& field, int64_t value,
return Status::InvalidArgument("Overflow");
}
*ret = ival + value;
char buf[32];
Int64ToStr(buf, 32, *ret);
batch.Put(handles_[1], hashes_data_key.Encode(), buf);
Int64ToStr(value_buf, 32, *ret);
batch.Put(handles_[1], hashes_data_key.Encode(), value_buf);
statistic++;
} else if (s.IsNotFound()) {
char buf[32];
Int64ToStr(buf, 32, value);
Int64ToStr(value_buf, 32, value);
parsed_hashes_meta_value.ModifyCount(1);
batch.Put(handles_[0], key, meta_value);
batch.Put(handles_[1], hashes_data_key.Encode(), buf);
batch.Put(handles_[1], hashes_data_key.Encode(), value_buf);
*ret = value;
} else {
return s;
}
}
} else if (s.IsNotFound()) {
char str[4];
EncodeFixed32(str, 1);
HashesMetaValue hashes_meta_value(std::string(str, sizeof(int32_t)));
EncodeFixed32(meta_value_buf, 1);
HashesMetaValue hashes_meta_value(Slice(meta_value_buf, sizeof(int32_t)));
version = hashes_meta_value.UpdateVersion();
batch.Put(handles_[0], key, hashes_meta_value.Encode());
HashesDataKey hashes_data_key(key, version, field);

char buf[32];
Int64ToStr(buf, 32, value);
batch.Put(handles_[1], hashes_data_key.Encode(), buf);
Int64ToStr(value_buf, 32, value);
batch.Put(handles_[1], hashes_data_key.Encode(), value_buf);
*ret = value;
} else {
return s;
Expand All @@ -393,6 +390,7 @@ Status RedisHashes::HIncrbyfloat(const Slice& key, const Slice& field, const Sli
}

Status s = db_->Get(default_read_options_, handles_[0], key, &meta_value);
char meta_value_buf[4] = {0};
if (s.ok()) {
ParsedHashesMetaValue parsed_hashes_meta_value(&meta_value);
if (parsed_hashes_meta_value.IsStale() || parsed_hashes_meta_value.count() == 0) {
Expand Down Expand Up @@ -431,9 +429,8 @@ Status RedisHashes::HIncrbyfloat(const Slice& key, const Slice& field, const Sli
}
}
} else if (s.IsNotFound()) {
char str[4];
EncodeFixed32(str, 1);
HashesMetaValue hashes_meta_value(std::string(str, sizeof(int32_t)));
EncodeFixed32(meta_value_buf, 1);
HashesMetaValue hashes_meta_value(Slice(meta_value_buf, sizeof(int32_t)));
version = hashes_meta_value.UpdateVersion();
batch.Put(handles_[0], key, hashes_meta_value.Encode());

Expand Down Expand Up @@ -559,6 +556,7 @@ Status RedisHashes::HMSet(const Slice& key, const std::vector<FieldValue>& fvs)
int32_t version = 0;
std::string meta_value;
Status s = db_->Get(default_read_options_, handles_[0], key, &meta_value);
char meta_value_buf[4] = {0};
if (s.ok()) {
ParsedHashesMetaValue parsed_hashes_meta_value(&meta_value);
if (parsed_hashes_meta_value.IsStale() || parsed_hashes_meta_value.count() == 0) {
Expand Down Expand Up @@ -590,9 +588,8 @@ Status RedisHashes::HMSet(const Slice& key, const std::vector<FieldValue>& fvs)
batch.Put(handles_[0], key, meta_value);
}
} else if (s.IsNotFound()) {
char str[4];
EncodeFixed32(str, filtered_fvs.size());
HashesMetaValue hashes_meta_value(std::string(str, sizeof(int32_t)));
EncodeFixed32(meta_value_buf, filtered_fvs.size());
HashesMetaValue hashes_meta_value(Slice(meta_value_buf, sizeof(int32_t)));
version = hashes_meta_value.UpdateVersion();
batch.Put(handles_[0], key, hashes_meta_value.Encode());
for (const auto& fv : filtered_fvs) {
Expand All @@ -613,6 +610,7 @@ Status RedisHashes::HSet(const Slice& key, const Slice& field, const Slice& valu
uint32_t statistic = 0;
std::string meta_value;
Status s = db_->Get(default_read_options_, handles_[0], key, &meta_value);
char meta_value_buf[4] = {0};
if (s.ok()) {
ParsedHashesMetaValue parsed_hashes_meta_value(&meta_value);
if (parsed_hashes_meta_value.IsStale() || parsed_hashes_meta_value.count() == 0) {
Expand Down Expand Up @@ -645,9 +643,8 @@ Status RedisHashes::HSet(const Slice& key, const Slice& field, const Slice& valu
}
}
} else if (s.IsNotFound()) {
char str[4];
EncodeFixed32(str, 1);
HashesMetaValue meta_value(Slice(str, sizeof(int32_t)));
EncodeFixed32(meta_value_buf, 1);
HashesMetaValue meta_value(Slice(meta_value_buf, sizeof(int32_t)));
version = meta_value.UpdateVersion();
batch.Put(handles_[0], key, meta_value.Encode());
HashesDataKey data_key(key, version, field);
Expand All @@ -668,6 +665,7 @@ Status RedisHashes::HSetnx(const Slice& key, const Slice& field, const Slice& va
int32_t version = 0;
std::string meta_value;
Status s = db_->Get(default_read_options_, handles_[0], key, &meta_value);
char meta_value_buf[4] = {0};
if (s.ok()) {
ParsedHashesMetaValue parsed_hashes_meta_value(&meta_value);
if (parsed_hashes_meta_value.IsStale() || parsed_hashes_meta_value.count() == 0) {
Expand All @@ -694,9 +692,8 @@ Status RedisHashes::HSetnx(const Slice& key, const Slice& field, const Slice& va
}
}
} else if (s.IsNotFound()) {
char str[4];
EncodeFixed32(str, 1);
HashesMetaValue hashes_meta_value(std::string(str, sizeof(int32_t)));
EncodeFixed32(meta_value_buf, 1);
HashesMetaValue hashes_meta_value(Slice(meta_value_buf, sizeof(int32_t)));
version = hashes_meta_value.UpdateVersion();
batch.Put(handles_[0], key, hashes_meta_value.Encode());
HashesDataKey hashes_data_key(key, version, field);
Expand Down
24 changes: 12 additions & 12 deletions src/storage/tests/lists_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
#include "src/redis.h"
#include "storage/storage.h"

using storage::Status;
using storage::Slice;
using storage::EncodeFixed64;
using storage::ListsMetaValue;
using storage::ListsDataFilter;
using storage::ListsDataKey;
using storage::ListsMetaValue;
using storage::Slice;
using storage::Status;

class ListsFilterTest : public ::testing::Test {
public:
Expand Down Expand Up @@ -77,7 +77,7 @@ TEST_F(ListsFilterTest, MetaFilterTest) {

// Timeout timestamp is not set, but it's an empty list.
EncodeFixed64(str, 0);
ListsMetaValue lists_meta_value1(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value1(Slice(str, sizeof(uint64_t)));
lists_meta_value1.UpdateVersion();
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
filter_result =
Expand All @@ -86,7 +86,7 @@ TEST_F(ListsFilterTest, MetaFilterTest) {

// Timeout timestamp is not set, it's not an empty list.
EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value2(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value2(Slice(str, sizeof(uint64_t)));
lists_meta_value2.UpdateVersion();
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
filter_result =
Expand All @@ -95,7 +95,7 @@ TEST_F(ListsFilterTest, MetaFilterTest) {

// Timeout timestamp is set, but not expired.
EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value3(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value3(Slice(str, sizeof(uint64_t)));
lists_meta_value3.UpdateVersion();
lists_meta_value3.SetRelativeTimestamp(3);
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
Expand All @@ -105,7 +105,7 @@ TEST_F(ListsFilterTest, MetaFilterTest) {

// Timeout timestamp is set, already expired.
EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value4(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value4(Slice(str, sizeof(uint64_t)));
lists_meta_value4.UpdateVersion();
lists_meta_value4.SetRelativeTimestamp(1);
std::this_thread::sleep_for(std::chrono::milliseconds(2000));
Expand All @@ -128,7 +128,7 @@ TEST_F(ListsFilterTest, DataFilterTest) {
ASSERT_TRUE(lists_data_filter1 != nullptr);

EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value1(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value1(Slice(str, sizeof(uint64_t)));
version = lists_meta_value1.UpdateVersion();
s = meta_db->Put(rocksdb::WriteOptions(), handles[0], "FILTER_TEST_KEY", lists_meta_value1.Encode());
ASSERT_TRUE(s.ok());
Expand All @@ -145,7 +145,7 @@ TEST_F(ListsFilterTest, DataFilterTest) {
ASSERT_TRUE(lists_data_filter2 != nullptr);

EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value2(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value2(Slice(str, sizeof(uint64_t)));
version = lists_meta_value2.UpdateVersion();
lists_meta_value2.SetRelativeTimestamp(1);
s = meta_db->Put(rocksdb::WriteOptions(), handles[0], "FILTER_TEST_KEY", lists_meta_value2.Encode());
Expand All @@ -162,7 +162,7 @@ TEST_F(ListsFilterTest, DataFilterTest) {
ASSERT_TRUE(lists_data_filter3 != nullptr);

EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value3(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value3(Slice(str, sizeof(uint64_t)));
version = lists_meta_value3.UpdateVersion();
lists_meta_value3.SetRelativeTimestamp(1);
s = meta_db->Put(rocksdb::WriteOptions(), handles[0], "FILTER_TEST_KEY", lists_meta_value3.Encode());
Expand All @@ -180,7 +180,7 @@ TEST_F(ListsFilterTest, DataFilterTest) {
ASSERT_TRUE(lists_data_filter4 != nullptr);

EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value4(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value4(Slice(str, sizeof(uint64_t)));
version = lists_meta_value4.UpdateVersion();
s = meta_db->Put(rocksdb::WriteOptions(), handles[0], "FILTER_TEST_KEY", lists_meta_value4.Encode());
ASSERT_TRUE(s.ok());
Expand All @@ -199,7 +199,7 @@ TEST_F(ListsFilterTest, DataFilterTest) {
ASSERT_TRUE(lists_data_filter5 != nullptr);

EncodeFixed64(str, 1);
ListsMetaValue lists_meta_value5(std::string(str, sizeof(uint64_t)));
ListsMetaValue lists_meta_value5(Slice(str, sizeof(uint64_t)));
version = lists_meta_value5.UpdateVersion();
s = meta_db->Put(rocksdb::WriteOptions(), handles[0], "FILTER_TEST_KEY", lists_meta_value5.Encode());
ASSERT_TRUE(s.ok());
Expand Down