Fix read from uninitialized memory

[Rot127]

Your checklist for this pull request

• I've documented or updated the documentation of every API function and struct this PR changes.
• I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

This PR fixes the uninitialized memory access in the ARM module (reported by @shaobaobaoer
in GHSA-8qp8-2vg2-8mr4).

The reported ARM bug was the following:

After a realloc() the uninitialized memory could be accessed by the ARM
module. It is not unlikely that other modules have similar bugs.
Or we could introduce some in the future, because the realloc() is
not in the mind of a module developer.

Hence, this commit simply zeros the bytes of the new memory.
That way we reduce the chance of future bugs.

Test plan

Added

Closing issues

...

[Copilot]

Pull request overview

This PR adds MemorySanitizer (MSAN) support to the build/CI setup and hardens Capstone against uninitialized-memory reads by zeroing newly realloc()’d instruction-cache memory. It also replaces nftw() usage in cstest with a custom directory traversal to avoid MSAN false positives, and expands integration tests to cover the reported ARM alias/POP case.

Changes:

• Add ENABLE_MSAN CMake option and propagate MSAN config through CI workflows.
• Zero newly extended cs_disasm() instruction-cache memory after realloc() to prevent uninitialized reads.
• Replace nftw() in cstest with a custom YAML test-file collector; add additional integration PoC tests and Valgrind-backed CTest entries.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
tests/integration/test_poc.c	Adds integration PoC tests for ARM POP alias/MSAN behavior and a TMS320 smoke check.
tests/integration/CMakeLists.txt	Switches integration tests to $<TARGET_FILE:...> and adds Valgrind variants.
suite/cstest/test/CMakeLists.txt	Adds a Valgrind variant of the unit_cstest test.
suite/cstest/src/cstest.c	Replaces nftw() with a custom recursive directory traversal for collecting .yaml/.yml tests.
suite/cstest/CMakeLists.txt	Adds Valgrind variants for the main cstest test groups.
Mapping.c	Initializes alias_id to 0 for non-alias instructions and adjusts alias-id handling paths.
cs.c	Zeroes newly reallocated instruction cache memory to avoid uninitialized reads.
CMakeLists.txt	Adds ENABLE_MSAN option and related sanitizer flags; adds ASAN/MSAN mutual exclusion.
.github/workflows/CrossBuilds.yml	Tightens ctest -R regex usage with anchors.
.github/workflows/CITest.yml	Adds MSAN env/options and extends the matrix/config plumbing for compiler + MSAN toggles; adjusts ctest -R regex patterns; routes Valgrind execution through CTest.

Suppressed comments (4)

suite/cstest/src/cstest.c:95

• When opendir() fails and path is a regular file, only .yaml is accepted. Passing a .yml file will be rejected even though the directory traversal path supports .yml. Also, the file-type check should use S_ISREG.

		if (check_path(dir, path, &sb) == 0 && sb.st_mode & S_IFREG &&
		    add_yaml_file_path(path, test_files, file_count, ".yaml")) {
			return true;
		}
		fprintf(stderr,
			"[!] '%s' is not a yaml file neither a directory.\n",
			path);
		return false;

suite/cstest/src/cstest.c:134

• fpath is allocated via cs_mem_calloc() but freed with free(). Use cs_mem_free() to match Capstone's allocator API.

		free(fpath);

suite/cstest/src/cstest.c:144

• The error path frees fpath with free(), but fpath is allocated by cs_mem_calloc(). Use cs_mem_free() so custom allocators work correctly.

error:
	free(fpath);
	closedir(d);
	return false;

tests/integration/test_poc.c:300

• This test helper is only used within this translation unit; keeping it non-static exports an unnecessary global symbol from the integration test binary.

void test_tms320_ghsa_8qp8_2vg2_8mr4(void)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

suite/cstest/src/cstest.c:52

• add_yaml_file_path() increments file_count before checking cs_mem_realloc() success. If realloc fails, file_count is left incremented while test_files remains NULL, which can later cause cleanup_test_files() to dereference test_files[0] out of bounds / NULL.

	(*file_count)++;
	*test_files = cs_mem_realloc(*test_files, sizeof(char *) * *file_count);
	if (!*test_files) {
		fprintf(stderr, "[!] realloc failed\n");
		return -1;
	}
	test_files[0][*file_count - 1] = cs_strdup(fpath);
	return 0;

suite/cstest/src/cstest.c:222

• cs_mem_malloc() return value is not checked before dereferencing (*test_files = NULL). If allocation fails, this will segfault instead of exiting cleanly.

	char ***test_files = cs_mem_malloc(sizeof(char **));
	*test_files = NULL;
	size_t file_count = 0;

tests/integration/CMakeLists.txt:47

• valgrind_integration_* tests are added unconditionally and hardcode the "valgrind" executable. This makes CTest fail in environments where valgrind is not installed or not on PATH (and also on non-Linux platforms).

  add_test(NAME "integration_${TBIN}" COMMAND $<TARGET_FILE:${TBIN}>)
  add_test(NAME "valgrind_integration_${TBIN}" COMMAND valgrind --leak-check=full --error-exitcode=1 $<TARGET_FILE:${TBIN}>)
endforeach()

suite/cstest/src/cstest.c:63

• On Windows, get_file_stat() calls _stat() but the error message says "wstat failed", which is misleading when debugging failures.

	if (_stat(fpath, sb) == -1) {
		fprintf(stderr, "[!] wstat failed.\n");
		return false;

suite/cstest/test/CMakeLists.txt:26

• The valgrind unit test is guarded by VALGRIND_BIN, but the COMMAND still hardcodes "valgrind" instead of using the discovered path. Using ${VALGRIND_BIN} avoids PATH-dependent failures.

if (VALGRIND_BIN)
add_test(NAME valgrind_unit_cstest
  COMMAND valgrind --leak-check=full --error-exitcode=1 $<TARGET_FILE:unit_test>
  WORKING_DIRECTORY ${CSTEST_TEST_DIR}
)
endif()

suite/cstest/CMakeLists.txt:181

• Valgrind tests are guarded by VALGRIND_BIN but still invoke "valgrind" via PATH. Prefer ${VALGRIND_BIN} (and $<TARGET_FILE:cstest>) for robustness and consistency with other tests.

if (VALGRIND_BIN)
add_test(NAME valgrind_MCTests
    COMMAND valgrind --leak-check=full --error-exitcode=1 cstest ${PROJECT_SOURCE_DIR}/tests/MC
    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
)
add_test(NAME valgrind_DetailTests
    COMMAND valgrind --leak-check=full --error-exitcode=1 cstest ${PROJECT_SOURCE_DIR}/tests/details
    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
)
add_test(NAME valgrind_IssueTests
    COMMAND valgrind --leak-check=full --error-exitcode=1 cstest ${PROJECT_SOURCE_DIR}/tests/issues
    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
)
add_test(NAME valgrind_FeaturesTests
    COMMAND valgrind --leak-check=full --error-exitcode=1 cstest ${PROJECT_SOURCE_DIR}/tests/features
    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
)

.github/workflows/CITest.yml:49

• PR title/description says a CI job runs tests with MSAN enabled, but the workflow matrix only defines enable-msan: 'OFF' configurations (and all use gcc). As-is, CI never exercises ENABLE_MSAN=ON.

          - {
              name: 'ubuntu-22.04 x64 make',
              os: ubuntu-22.04,
              arch: x64,
              build-system: 'make',
              diet-build: 'OFF',
              enable-asan: 'OFF',
              enable-msan: 'OFF',
              cc: 'gcc'
            }

tests/integration/test_poc.c:300

• test_tms320_ghsa_8qp8_2vg2_8mr4() is the only newly added non-static test helper in this file, but it is only used within this translation unit. Making it static avoids exporting an unnecessary global symbol and matches the surrounding test_* helpers.

/// Shouldn't trigger MSAN. Just added here to check.
void test_tms320_ghsa_8qp8_2vg2_8mr4(void)
{

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

cs.c:1

• The new memset(total_size - old_total_size) makes any integer overflow in sizeof(cs_insn) * cache_size or the subsequent total_size += ... immediately dangerous (underflow/huge length), potentially causing an out-of-bounds write. Please add explicit overflow checks before updating total_size (e.g., validate the multiplication and that old_total_size + added does not exceed SIZE_MAX) and fail with CS_ERR_MEM if the new size can’t be represented.

/* Capstone Disassembly Engine */

tests/integration/test_poc.c:291

• These tests rely on assert(0) to fail, but with NDEBUG the assert is compiled out and the test will silently return, potentially masking failures (e.g., if an arch is disabled in the build). Consider making failures unconditional for test binaries (e.g., fprintf + abort()/exit(1), or use an assertion that remains effective in release test configurations used in CI).

	if (cs_open(CS_ARCH_ARM, CS_MODE_ARM, &h) != CS_ERR_OK) {
		assert(0);
		return;
	}

	if (cs_option(h, CS_OPT_DETAIL, CS_OPT_ON) != CS_ERR_OK) {
		assert(0);
		return;
	}

tests/integration/test_poc.c:312

• These tests rely on assert(0) to fail, but with NDEBUG the assert is compiled out and the test will silently return, potentially masking failures (e.g., if an arch is disabled in the build). Consider making failures unconditional for test binaries (e.g., fprintf + abort()/exit(1), or use an assertion that remains effective in release test configurations used in CI).

	if (cs_open(CS_ARCH_TMS320C64X, CS_MODE_BIG_ENDIAN, &h) != CS_ERR_OK) {
		assert(0);
		return;
	}

	if (cs_option(h, CS_OPT_DETAIL, CS_OPT_ON) != CS_ERR_OK) {
		assert(0);
		return;
	}

tests/integration/test_poc.c:299

• This comment is a bit ambiguous about what is being verified and under which build/run conditions (MSAN-enabled builds only vs. always). Consider clarifying the intent (e.g., 'Regression test for GHSA-... under MSAN: should not report uninitialized read when disassembling ...').

/// Shouldn't trigger MSAN. Just added here to check.