fix address sanitize reported stack-use-after-scope

[tedli]

fix #1711

[cr-gpt]

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.

[tedli]

如 #1711 里 @cheniujh 提到的。

pika/src/storage/src/redis_hashes.cc

Lines 320 to 375 in 3e745f3

	if (s.ok()) {
	ParsedHashesMetaValue parsed_hashes_meta_value(&meta_value);
	if (parsed_hashes_meta_value.IsStale() || parsed_hashes_meta_value.count() == 0) {
	version = parsed_hashes_meta_value.UpdateVersion();
	parsed_hashes_meta_value.set_count(1);
	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);
	*ret = value;
	} else {
	version = parsed_hashes_meta_value.version();
	HashesDataKey hashes_data_key(key, version, field);
	s = db_->Get(default_read_options_, handles_[1], hashes_data_key.Encode(), &old_value);
	if (s.ok()) {
	int64_t ival = 0;
	if (StrToInt64(old_value.data(), old_value.size(), &ival) == 0) {
	return Status::Corruption("hash value is not an integer");
	}
	if ((value >= 0 && LLONG_MAX - value < ival) || (value < 0 && LLONG_MIN - value > ival)) {
	return Status::InvalidArgument("Overflow");
	}
	*ret = ival + value;
	char buf[32];
	Int64ToStr(buf, 32, *ret);
	batch.Put(handles_[1], hashes_data_key.Encode(), buf);
	statistic++;
	} else if (s.IsNotFound()) {
	char buf[32];
	Int64ToStr(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);
	*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)));
	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);
	*ret = value;
	} else {
	return s;
	}
	s = db_->Write(default_write_options_, &batch);

这段代码里，各种 if、else 判断，在 if、else 块里，有如 328、345、350、361 行的临时 char buf[]，然后向 batch 里加内容，这些内容，包含了对临时 char buf[] 地址的引用。

而 batch 最终执行写操作是在 if、else 块外，已经出了各个 char buf[] 的上下文，这时的 write batch 写的内容是未定义行为。

这个问题，只要把 char buf[] 的作用域挪到跟 batch 一个级就可以解决了。说明 sanitizer 还挺智能的，帮找到一个潜在 bug。

这个改动可以修复 hashes_test、keys_test。

但是，lists_filter_test 还是 sanitize 不过，只不过 lists_filter_test 这回报的是 test case 代码里导致的。而且检查了一遍，没发现有出作用域这个问题。而且 sanitizer 报的内容也说了这么一句：

意思是说这可能是个“误报”。

[tedli]

https://learn.microsoft.com/en-us/cpp/sanitizers/error-stack-use-after-scope?view=msvc-170#example-3---destructor-ordering-with-locals

这个文章，把 stack-use-after-scope 可能的几种情况，讲得比较明了，其中 lists_filter_test 应该不是误报，应该是踩了文章里的情况 4。

我找时间再修修。

[tedli]

lists_filter_test 也修了。这个问题是实例化 ListsMetaValue 时，参数用的 std::string，是个右值，虽然给 string 的 ctor 的参是 char str[8]，但是 string 的 ctor 里实现逻辑是拷贝行为，就是引用的不是 str，而是 ctor 里动态分配的一块空间，而这个动态分配空间，会随 string 对象本身 dtor 被释放，而用到的又是个右值，所以肯定是被 dtor 掉了，这时 lists_meta_value l里引用的实际上是被 string 释放掉的内存空间。如第 80 行：

pika/src/storage/tests/lists_filter_test.cc

Lines 67 to 81 in 3e745f3

	TEST_F(ListsFilterTest, MetaFilterTest) {
	char str[8];
	bool filter_result;
	bool value_changed;
	int32_t version = 0;
	std::string new_value;
	// Test Meta Filter
	auto lists_meta_filter = std::make_unique<storage::ListsMetaFilter>();
	ASSERT_TRUE(lists_meta_filter != nullptr);
	// Timeout timestamp is not set, but it's an empty list.
	EncodeFixed64(str, 0);
	ListsMetaValue lists_meta_value1(std::string(str, sizeof(uint64_t)));
	lists_meta_value1.UpdateVersion();

[tedli]

sanitizer 找的问题还挺准，建议全局都用 sanitizer 验一下。@AlexStocks

[Mixficsol]

sanitizer 找的问题还挺准，建议全局都用 sanitizer 验一下。@AlexStocks

你好，我最近在CI上加了Macos环境的测试，其中CTest测试是跑不过的，如你的这个提交，如果你最后在本机检测完毕没问题了可以在pika.yml帮我把这个注释取消掉吗，这样可以帮你检测Macos环境下这个PR能否通过

这个是PR的地址：PR

[tedli]

Hi @Mixficsol ,

这个 issue 最初提到的几个问题，我都修了。然后我本地是可以跑过 test 的。

不过这个 pr 的 ci 我才看到，build 失败了，这个挺奇怪，build_on_centos 倒是成功的，这个我本地再找找啥原因。不行的话，我就给你说的那块注释打开，重推一下，触发重新跑 ci 看看还能通过不。

[cr-gpt]

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.

[tedli]

我跟之前的 pr 一行没改，只是把 2 个 commit 给 rebase 成 1 个 commit，重新 push 了一下，触发重新 build。
这次 build 成功了，但是 build_on_macos 是失败的，应该是构建脚本的问题。

[Mixficsol]

Hi @tedli,
现在CI有三个环境，一个是ubuntu的就是(build)，一个是跑在Docker里面的centos环境，还有一个就是我刚加的Macos，目前Macos下的Ctest是跑不过的，不知道你本地是Macos环境吗，如果是的话，你本地能跑过应该CI这部分也能跑过的

[tedli]

Hi @tedli, 现在CI有三个环境，一个是ubuntu的就是(build)，一个是跑在Docker里面的centos环境，还有一个就是我刚加的Macos，目前Macos下的Ctest是跑不过的，不知道你本地是Macos环境吗，如果是的话，你本地能跑过应该CI这部分也能跑过的

我看 build log 是没找到 c compiler，都没开始构建，就退了。应该跟这个 pr 代码改动无关。

[Mixficsol]

Hi @tedli, 现在CI有三个环境，一个是ubuntu的就是(build)，一个是跑在Docker里面的centos环境，还有一个就是我刚加的Macos，目前Macos下的Ctest是跑不过的，不知道你本地是Macos环境吗，如果是的话，你本地能跑过应该CI这部分也能跑过的

我看 build log 是没找到 c compiler，都没开始构建，就退了。应该跟这个 pr 代码改动无关。

现在可以pull一下代码没问题了，之前有一个PR
这个导致下载依赖直接会跳过，目前我把这个删除掉了，你可以再试试

[cr-gpt]

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.