From b93ed6d0e13c711c10d95b240c8b4ae637a768c6 Mon Sep 17 00:00:00 2001 From: "A bot of @njzjz" <48687836+njzjz-bot@users.noreply.github.com> Date: Sat, 11 Jul 2026 06:54:20 +0800 Subject: [PATCH 1/3] fix(api_c): preserve exact bytes in DP_ReadFileToChar2 DP_ReadFileToChar2 reported the original file size but returned a buffer from string_to_char, which trims trailing whitespace before allocating. The C++ wrapper then reconstructed a std::string with the reported (larger) size, causing an over-read of the shorter allocation. Fix by adding string_to_char_exact, a helper that preserves every byte without trimming, and use it in DP_ReadFileToChar2 and DP_ReadFileToChar. Error-message paths still use the trimming string_to_char since whitespace trimming is desirable there. Added both a C++ test (test_read_file_to_string.cc) and a Python test (test_c_api_readfile.py) that verify trailing whitespace is preserved. Fixes #5620. Generated with opencode using model glm-5.2. --- source/api_c/src/c_api.cc | 31 +++++- .../api_cc/tests/test_read_file_to_string.cc | 32 ++++++ source/tests/test_c_api_readfile.py | 102 ++++++++++++++++++ 3 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 source/tests/test_c_api_readfile.py diff --git a/source/api_c/src/c_api.cc b/source/api_c/src/c_api.cc index 1346bfbf20..9e9c7c600a 100644 --- a/source/api_c/src/c_api.cc +++ b/source/api_c/src/c_api.cc @@ -1535,6 +1535,27 @@ const char* string_to_char(std::string& str) { return buffer; } +/** + * @brief Convert std::string to const char without trimming. + * + * Unlike string_to_char, this helper preserves every byte of the input, + * including trailing whitespace. This is necessary for file-reading + * functions (e.g. DP_ReadFileToChar2) where the reported size must + * match the allocated buffer exactly; see issue #5620. + * + * @param[in] str std::string to be converted (not modified) + * @return const char* heap-allocated buffer, caller must DP_DeleteChar + */ +const char* string_to_char_exact(const std::string& str) { + // copy from string to char* without any trimming + const std::string::size_type size = str.size(); + // +1 for '\0' + char* buffer = new char[size + 1]; + std::copy(str.begin(), str.end(), buffer); + buffer[size] = '\0'; + return buffer; +} + extern "C" { const char* DP_NlistCheckOK(DP_Nlist* nlist) { @@ -2642,7 +2663,8 @@ const char* DP_ReadFileToChar(const char* c_model) { std::string model(c_model); std::string file_content; deepmd::read_file_to_string(model, file_content); - return string_to_char(file_content); + // Preserve exact bytes — see issue #5620 for why trimming is wrong here. + return string_to_char_exact(file_content); } const char* DP_ReadFileToChar2(const char* c_model, int* size) { @@ -2656,8 +2678,13 @@ const char* DP_ReadFileToChar2(const char* c_model, int* size) { *size = -error_message.size(); return string_to_char(error_message); } + // Record the exact file size before any conversion. We must use + // string_to_char_exact (not string_to_char) so that trailing + // whitespace is preserved and the returned buffer has exactly *size + // bytes — otherwise the C++ wrapper would reconstruct a string that + // over-reads the shorter allocation. See issue #5620. *size = file_content.size(); - return string_to_char(file_content); + return string_to_char_exact(file_content); } void DP_SelectByType(const int natoms, diff --git a/source/api_cc/tests/test_read_file_to_string.cc b/source/api_cc/tests/test_read_file_to_string.cc index dd3249d771..186dc37aa8 100644 --- a/source/api_cc/tests/test_read_file_to_string.cc +++ b/source/api_cc/tests/test_read_file_to_string.cc @@ -26,3 +26,35 @@ TEST(TestReadFileToString, readfiletostring) { std::string expected_out_string = buffer.str(); EXPECT_STREQ(expected_out_string.c_str(), file_content.c_str()); } + +// Regression test for issue #5620: DP_ReadFileToChar2 must preserve +// exact file bytes including trailing whitespace, and the reported size +// must match the allocated buffer. +TEST(TestReadFileToString, readfiletostring_exact_bytes) { + // Write a temporary file whose content ends with " \n" (trailing + // whitespace). The bug was that string_to_char trimmed this + // whitespace but the size still reported the original length, + // causing an over-read when the C++ wrapper reconstructed a string + // with the reported size. + std::string tmp_file = "test_readfile_exact_bytes.txt"; + std::string content = "hello world \n"; // ends with space + newline + { + std::ofstream ofs(tmp_file); + ofs << content; + ofs.close(); + } + + // Read through the C++ wrapper which calls DP_ReadFileToChar2 + std::string file_content; + deepmd::read_file_to_string(tmp_file, file_content); + + // The full content must be preserved byte-for-byte. + EXPECT_EQ(content.size(), file_content.size()) + << "Size mismatch: the file has " << content.size() + << " bytes but read_file_to_string returned " << file_content.size(); + EXPECT_EQ(content, file_content) + << "Content mismatch: trailing whitespace was not preserved"; + + // Clean up the temporary file. + std::remove(tmp_file.c_str()); +} diff --git a/source/tests/test_c_api_readfile.py b/source/tests/test_c_api_readfile.py new file mode 100644 index 0000000000..495b9b12fa --- /dev/null +++ b/source/tests/test_c_api_readfile.py @@ -0,0 +1,102 @@ +# SPDX-License-Identifier: LGPL-3.0-or-later +"""Regression test for ``DP_ReadFileToChar2`` / ``read_file_to_string``. + +See https://github.com/deepmodeling/deepmd-kit/issues/5620 for context. +Previously ``DP_ReadFileToChar2`` reported the original file size but +returned a buffer produced by ``string_to_char``, which trims trailing +whitespace before allocating/copying. The C++ wrapper then reconstructed a +``std::string`` with the reported (larger) size, causing an over-read of the +shorter allocation. This test verifies that exact bytes are preserved, +including trailing whitespace. +""" + +import ctypes +import pathlib + +import pytest + + +def _load_c_lib(): + """Load the DeePMD C library (``libdeepmd_c.so``). + + The library ships with the Python package; we search the standard + locations and fall back to the build tree. + """ + import deepmd + + candidates = [ + pathlib.Path(deepmd.__file__).parent / "lib" / "libdeepmd_c.so", + pathlib.Path(__file__).resolve().parents[2] / "dp" / "lib" / "libdeepmd_c.so", + ] + for path in candidates: + if path.exists(): + return ctypes.CDLL(str(path)) + pytest.skip("libdeepmd_c.so not found") + + +def test_readfiletochar2_preserves_trailing_whitespace(tmp_path): + """Files ending in whitespace must return exact size and bytes.""" + lib = _load_c_lib() + + # Set up function signatures ------------------------------------------- + # Use c_void_p for the return type so ctypes does not truncate the + # buffer at the first null byte (c_char_p would do that). + lib.DP_ReadFileToChar2.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_int)] + lib.DP_ReadFileToChar2.restype = ctypes.c_void_p + lib.DP_DeleteChar.argtypes = [ctypes.c_void_p] + lib.DP_DeleteChar.restype = None + + # Write a temporary file whose content ends with " \n" (trailing + # whitespace). This is the exact scenario that was broken: the old + # code trimmed the whitespace but reported the original size. + content = b"hello world \n" # ends with space + newline + tmp_file = tmp_path / "test_exact_bytes.txt" + tmp_file.write_bytes(content) + + # Call DP_ReadFileToChar2 ---------------------------------------------- + size = ctypes.c_int(0) + c_buf = lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) + assert c_buf is not None, "DP_ReadFileToChar2 returned NULL" + + size_val = size.value + # Negative size indicates an error (the error message is in the buffer). + assert size_val >= 0, ( + f"DP_ReadFileToChar2 error: {ctypes.string_at(c_buf, -size_val)}" + ) + + # The reported size must match the file size exactly. + assert size_val == len(content), ( + f"Size mismatch: file has {len(content)} bytes but " + f"DP_ReadFileToChar2 reported {size_val}" + ) + + # The returned buffer must contain the exact file bytes, including + # trailing whitespace. + returned_bytes = ctypes.string_at(c_buf, size_val) + assert returned_bytes == content, ( + f"Content mismatch: expected {content!r} but got {returned_bytes!r}" + ) + + # Clean up the allocated buffer. + lib.DP_DeleteChar(c_buf) + + +def test_readfiletochar2_preserves_no_trailing_whitespace(tmp_path): + """Sanity check: files without trailing whitespace still work.""" + lib = _load_c_lib() + # Use c_void_p for the return type so ctypes does not truncate the + # buffer at the first null byte (c_char_p would do that). + lib.DP_ReadFileToChar2.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_int)] + lib.DP_ReadFileToChar2.restype = ctypes.c_void_p + lib.DP_DeleteChar.argtypes = [ctypes.c_void_p] + lib.DP_DeleteChar.restype = None + + content = b"hello world" + tmp_file = tmp_path / "test_no_trailing.txt" + tmp_file.write_bytes(content) + + size = ctypes.c_int(0) + c_buf = lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) + assert size.value == len(content) + assert ctypes.string_at(c_buf, size.value) == content + lib.DP_DeleteChar(c_buf) From f7cd7600000c1bf4a7122c01243e0bb351ea6ae4 Mon Sep 17 00:00:00 2001 From: njzjz-bot Date: Sat, 11 Jul 2026 13:56:20 +0800 Subject: [PATCH 2/3] fix(api_c): address review comments for PR #5763 - Add INT_MAX overflow guard in DP_ReadFileToChar2 - Correct multi-frame buffer documentation for charge-spin DeepPot APIs - Refactor test_c_api_readfile.py to use a module-scoped pytest fixture - Fix implicit return in _load_c_lib (CodeQL/Ruff) Coding-Agent: opencode opencode-Version: 1.17.18 Model: ustc/deepseek-v4-pro Reasoning-Effort: max Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh --- source/api_c/include/c_api.h | 76 +++++++++++++++-------------- source/api_c/src/c_api.cc | 8 ++- source/tests/test_c_api_readfile.py | 41 ++++++++-------- 3 files changed, 68 insertions(+), 57 deletions(-) diff --git a/source/api_c/include/c_api.h b/source/api_c/include/c_api.h index 31b9e0626c..64585f58b6 100644 --- a/source/api_c/include/c_api.h +++ b/source/api_c/include/c_api.h @@ -682,11 +682,11 @@ extern void DP_DeepPotComputeNListf2(DP_DeepPot* dp, * @param[in] dp The DP to use. * @param[in] nframes The number of frames. * @param[in] natoms The number of atoms. - * @param[in] coord The coordinates of atoms. The array should be of size natoms - *x 3. + * @param[in] coord The coordinates of atoms. The array should be of size + *nframes x natoms x 3. * @param[in] atype The atom types. The array should contain natoms ints. - * @param[in] cell The cell of the region. The array should be of size 9. Pass - *NULL if pbc is not used. + * @param[in] cell The cell of the region. The array should be of size nframes x + *9. Pass NULL if pbc is not used. * @param[in] fparam The frame parameters. The array can be of size nframes x *dim_fparam. * @param[in] aparam The atom parameters. The array can be of size nframes x @@ -694,13 +694,14 @@ extern void DP_DeepPotComputeNListf2(DP_DeepPot* dp, * @param[in] charge_spin The per-frame charge/spin input. The array can be of *size nframes x dim_chg_spin. Pass NULL to use the model's stored *default_chg_spin. - * @param[out] energy Output energy. - * @param[out] force Output force. The array should be of size natoms x 3. - * @param[out] virial Output virial. The array should be of size 9. + * @param[out] energy Output energy. The array should be of size nframes. + * @param[out] force Output force. The array should be of size nframes x natoms + *x 3. + * @param[out] virial Output virial. The array should be of size nframes x 9. * @param[out] atomic_energy Output atomic energy. The array should be of size - *natoms. + *nframes x natoms. * @param[out] atomic_virial Output atomic virial. The array should be of size - *natoms x 9. + *nframes x natoms x 9. * @warning The output arrays should be allocated before calling this function. *Pass NULL if not required. * @since API version 27 @@ -727,11 +728,11 @@ extern void DP_DeepPotCompute3(DP_DeepPot* dp, * @param[in] dp The DP to use. * @param[in] nframes The number of frames. * @param[in] natoms The number of atoms. - * @param[in] coord The coordinates of atoms. The array should be of size natoms - *x 3. + * @param[in] coord The coordinates of atoms. The array should be of size + *nframes x natoms x 3. * @param[in] atype The atom types. The array should contain natoms ints. - * @param[in] cell The cell of the region. The array should be of size 9. Pass - *NULL if pbc is not used. + * @param[in] cell The cell of the region. The array should be of size nframes x + *9. Pass NULL if pbc is not used. * @param[in] fparam The frame parameters. The array can be of size nframes x *dim_fparam. * @param[in] aparam The atom parameters. The array can be of size nframes x @@ -739,13 +740,14 @@ extern void DP_DeepPotCompute3(DP_DeepPot* dp, * @param[in] charge_spin The per-frame charge/spin input. The array can be of *size nframes x dim_chg_spin. Pass NULL to use the model's stored *default_chg_spin. - * @param[out] energy Output energy. - * @param[out] force Output force. The array should be of size natoms x 3. - * @param[out] virial Output virial. The array should be of size 9. + * @param[out] energy Output energy. The array should be of size nframes. + * @param[out] force Output force. The array should be of size nframes x natoms + *x 3. + * @param[out] virial Output virial. The array should be of size nframes x 9. * @param[out] atomic_energy Output atomic energy. The array should be of size - *natoms. + *nframes x natoms. * @param[out] atomic_virial Output atomic virial. The array should be of size - *natoms x 9. + *nframes x natoms x 9. * @warning The output arrays should be allocated before calling this function. *Pass NULL if not required. * @since API version 27 @@ -772,11 +774,11 @@ extern void DP_DeepPotComputef3(DP_DeepPot* dp, * @param[in] dp The DP to use. * @param[in] nframes The number of frames. * @param[in] natoms The number of atoms. - * @param[in] coord The coordinates of atoms. The array should be of size natoms - *x 3. + * @param[in] coord The coordinates of atoms. The array should be of size + *nframes x natoms x 3. * @param[in] atype The atom types. The array should contain natoms ints. - * @param[in] cell The cell of the region. The array should be of size 9. Pass - *NULL if pbc is not used. + * @param[in] cell The cell of the region. The array should be of size nframes x + *9. Pass NULL if pbc is not used. * @param[in] nghost The number of ghost atoms. * @param[in] nlist The neighbor list. * @param[in] ago Update the internal neighbour list if ago is 0. @@ -787,13 +789,14 @@ extern void DP_DeepPotComputef3(DP_DeepPot* dp, * @param[in] charge_spin The per-frame charge/spin input. The array can be of *size nframes x dim_chg_spin. Pass NULL to use the model's stored *default_chg_spin. - * @param[out] energy Output energy. - * @param[out] force Output force. The array should be of size natoms x 3. - * @param[out] virial Output virial. The array should be of size 9. + * @param[out] energy Output energy. The array should be of size nframes. + * @param[out] force Output force. The array should be of size nframes x natoms + *x 3. + * @param[out] virial Output virial. The array should be of size nframes x 9. * @param[out] atomic_energy Output atomic energy. The array should be of size - *natoms. + *nframes x natoms. * @param[out] atomic_virial Output atomic virial. The array should be of size - *natoms x 9. + *nframes x natoms x 9. * @warning The output arrays should be allocated before calling this function. *Pass NULL if not required. * @since API version 27 @@ -823,11 +826,11 @@ extern void DP_DeepPotComputeNList3(DP_DeepPot* dp, * @param[in] dp The DP to use. * @param[in] nframes The number of frames. * @param[in] natoms The number of atoms. - * @param[in] coord The coordinates of atoms. The array should be of size natoms - *x 3. + * @param[in] coord The coordinates of atoms. The array should be of size + *nframes x natoms x 3. * @param[in] atype The atom types. The array should contain natoms ints. - * @param[in] cell The cell of the region. The array should be of size 9. Pass - *NULL if pbc is not used. + * @param[in] cell The cell of the region. The array should be of size nframes x + *9. Pass NULL if pbc is not used. * @param[in] nghost The number of ghost atoms. * @param[in] nlist The neighbor list. * @param[in] ago Update the internal neighbour list if ago is 0. @@ -838,13 +841,14 @@ extern void DP_DeepPotComputeNList3(DP_DeepPot* dp, * @param[in] charge_spin The per-frame charge/spin input. The array can be of *size nframes x dim_chg_spin. Pass NULL to use the model's stored *default_chg_spin. - * @param[out] energy Output energy. - * @param[out] force Output force. The array should be of size natoms x 3. - * @param[out] virial Output virial. The array should be of size 9. + * @param[out] energy Output energy. The array should be of size nframes. + * @param[out] force Output force. The array should be of size nframes x natoms + *x 3. + * @param[out] virial Output virial. The array should be of size nframes x 9. * @param[out] atomic_energy Output atomic energy. The array should be of size - *natoms. + *nframes x natoms. * @param[out] atomic_virial Output atomic virial. The array should be of size - *natoms x 9. + *nframes x natoms x 9. * @warning The output arrays should be allocated before calling this function. *Pass NULL if not required. * @since API version 27 diff --git a/source/api_c/src/c_api.cc b/source/api_c/src/c_api.cc index 9e9c7c600a..b520928581 100644 --- a/source/api_c/src/c_api.cc +++ b/source/api_c/src/c_api.cc @@ -2683,7 +2683,13 @@ const char* DP_ReadFileToChar2(const char* c_model, int* size) { // whitespace is preserved and the returned buffer has exactly *size // bytes — otherwise the C++ wrapper would reconstruct a string that // over-reads the shorter allocation. See issue #5620. - *size = file_content.size(); + if (file_content.size() > INT_MAX) { + std::string error_message = + "File is too large to be read into a char buffer via this API"; + *size = -static_cast(error_message.size()); + return string_to_char(error_message); + } + *size = static_cast(file_content.size()); return string_to_char_exact(file_content); } diff --git a/source/tests/test_c_api_readfile.py b/source/tests/test_c_api_readfile.py index 495b9b12fa..3d2b7b32b7 100644 --- a/source/tests/test_c_api_readfile.py +++ b/source/tests/test_c_api_readfile.py @@ -32,20 +32,27 @@ def _load_c_lib(): if path.exists(): return ctypes.CDLL(str(path)) pytest.skip("libdeepmd_c.so not found") + return None -def test_readfiletochar2_preserves_trailing_whitespace(tmp_path): - """Files ending in whitespace must return exact size and bytes.""" +@pytest.fixture(scope="module") +def c_lib(): + """Module-scoped fixture that loads the C library and configures + the DP_ReadFileToChar2 / DP_DeleteChar signatures once. + """ lib = _load_c_lib() - - # Set up function signatures ------------------------------------------- - # Use c_void_p for the return type so ctypes does not truncate the - # buffer at the first null byte (c_char_p would do that). - lib.DP_ReadFileToChar2.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_int)] + lib.DP_ReadFileToChar2.argtypes = [ + ctypes.c_char_p, + ctypes.POINTER(ctypes.c_int), + ] lib.DP_ReadFileToChar2.restype = ctypes.c_void_p lib.DP_DeleteChar.argtypes = [ctypes.c_void_p] lib.DP_DeleteChar.restype = None + return lib + +def test_readfiletochar2_preserves_trailing_whitespace(tmp_path, c_lib): + """Files ending in whitespace must return exact size and bytes.""" # Write a temporary file whose content ends with " \n" (trailing # whitespace). This is the exact scenario that was broken: the old # code trimmed the whitespace but reported the original size. @@ -54,8 +61,10 @@ def test_readfiletochar2_preserves_trailing_whitespace(tmp_path): tmp_file.write_bytes(content) # Call DP_ReadFileToChar2 ---------------------------------------------- + # Use c_void_p for the return type so ctypes does not truncate the + # buffer at the first null byte (c_char_p would do that). size = ctypes.c_int(0) - c_buf = lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) + c_buf = c_lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) assert c_buf is not None, "DP_ReadFileToChar2 returned NULL" size_val = size.value @@ -78,25 +87,17 @@ def test_readfiletochar2_preserves_trailing_whitespace(tmp_path): ) # Clean up the allocated buffer. - lib.DP_DeleteChar(c_buf) + c_lib.DP_DeleteChar(c_buf) -def test_readfiletochar2_preserves_no_trailing_whitespace(tmp_path): +def test_readfiletochar2_preserves_no_trailing_whitespace(tmp_path, c_lib): """Sanity check: files without trailing whitespace still work.""" - lib = _load_c_lib() - # Use c_void_p for the return type so ctypes does not truncate the - # buffer at the first null byte (c_char_p would do that). - lib.DP_ReadFileToChar2.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_int)] - lib.DP_ReadFileToChar2.restype = ctypes.c_void_p - lib.DP_DeleteChar.argtypes = [ctypes.c_void_p] - lib.DP_DeleteChar.restype = None - content = b"hello world" tmp_file = tmp_path / "test_no_trailing.txt" tmp_file.write_bytes(content) size = ctypes.c_int(0) - c_buf = lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) + c_buf = c_lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) assert size.value == len(content) assert ctypes.string_at(c_buf, size.value) == content - lib.DP_DeleteChar(c_buf) + c_lib.DP_DeleteChar(c_buf) From 223f88f948a3037a87aa3f265ac88a7767fca0b9 Mon Sep 17 00:00:00 2001 From: njzjz-bot Date: Sun, 12 Jul 2026 17:40:26 +0800 Subject: [PATCH 3/3] fix(api_c): move read-file regression to C API tests Test exact byte preservation through the native C API suite instead of ctypes in the Python tests. Use std::numeric_limits for the file-size guard so GCC and Clang builds compile portably. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh --- source/api_c/src/c_api.cc | 4 +- .../api_c/tests/test_read_file_to_string.cc | 42 +++++++ .../api_cc/tests/test_read_file_to_string.cc | 32 ------ source/tests/test_c_api_readfile.py | 103 ------------------ 4 files changed, 45 insertions(+), 136 deletions(-) delete mode 100644 source/tests/test_c_api_readfile.py diff --git a/source/api_c/src/c_api.cc b/source/api_c/src/c_api.cc index b520928581..3047688b8a 100644 --- a/source/api_c/src/c_api.cc +++ b/source/api_c/src/c_api.cc @@ -1,6 +1,7 @@ // SPDX-License-Identifier: LGPL-3.0-or-later #include "c_api.h" +#include #include #include #include @@ -2683,7 +2684,8 @@ const char* DP_ReadFileToChar2(const char* c_model, int* size) { // whitespace is preserved and the returned buffer has exactly *size // bytes — otherwise the C++ wrapper would reconstruct a string that // over-reads the shorter allocation. See issue #5620. - if (file_content.size() > INT_MAX) { + if (file_content.size() > + static_cast(std::numeric_limits::max())) { std::string error_message = "File is too large to be read into a char buffer via this API"; *size = -static_cast(error_message.size()); diff --git a/source/api_c/tests/test_read_file_to_string.cc b/source/api_c/tests/test_read_file_to_string.cc index bef2a28da0..f72a832e0a 100644 --- a/source/api_c/tests/test_read_file_to_string.cc +++ b/source/api_c/tests/test_read_file_to_string.cc @@ -6,11 +6,14 @@ #include #include +#include #include +#include #include #include #include +#include "c_api.h" #include "deepmd.hpp" TEST(TestReadFileToString, readfiletostring) { #ifndef BUILD_TENSORFLOW @@ -40,3 +43,42 @@ TEST(TestReadFileToString, readfiletostringerr) { }, deepmd::hpp::deepmd_exception); } + +TEST(TestReadFileToString, c_api_preserves_exact_bytes) { + const std::string file_name = "test_read_file_to_char_exact_bytes.bin"; + const std::string expected("hello\0world \n", 13); + { + std::ofstream output(file_name, std::ios::binary); + ASSERT_TRUE(output.is_open()); + output.write(expected.data(), expected.size()); + } + + int size = 0; + std::unique_ptr content( + DP_ReadFileToChar2(file_name.c_str(), &size), DP_DeleteChar); + + ASSERT_NE(content, nullptr); + ASSERT_GE(size, 0); + EXPECT_EQ(size, expected.size()); + EXPECT_EQ(std::string(content.get(), size), expected); + + EXPECT_EQ(std::remove(file_name.c_str()), 0); +} + +TEST(TestReadFileToString, legacy_c_api_preserves_trailing_whitespace) { + const std::string file_name = "test_read_file_to_char_whitespace.txt"; + const std::string expected = "hello world \n"; + { + std::ofstream output(file_name, std::ios::binary); + ASSERT_TRUE(output.is_open()); + output.write(expected.data(), expected.size()); + } + + std::unique_ptr content( + DP_ReadFileToChar(file_name.c_str()), DP_DeleteChar); + + ASSERT_NE(content, nullptr); + EXPECT_EQ(std::string(content.get()), expected); + + EXPECT_EQ(std::remove(file_name.c_str()), 0); +} diff --git a/source/api_cc/tests/test_read_file_to_string.cc b/source/api_cc/tests/test_read_file_to_string.cc index 186dc37aa8..dd3249d771 100644 --- a/source/api_cc/tests/test_read_file_to_string.cc +++ b/source/api_cc/tests/test_read_file_to_string.cc @@ -26,35 +26,3 @@ TEST(TestReadFileToString, readfiletostring) { std::string expected_out_string = buffer.str(); EXPECT_STREQ(expected_out_string.c_str(), file_content.c_str()); } - -// Regression test for issue #5620: DP_ReadFileToChar2 must preserve -// exact file bytes including trailing whitespace, and the reported size -// must match the allocated buffer. -TEST(TestReadFileToString, readfiletostring_exact_bytes) { - // Write a temporary file whose content ends with " \n" (trailing - // whitespace). The bug was that string_to_char trimmed this - // whitespace but the size still reported the original length, - // causing an over-read when the C++ wrapper reconstructed a string - // with the reported size. - std::string tmp_file = "test_readfile_exact_bytes.txt"; - std::string content = "hello world \n"; // ends with space + newline - { - std::ofstream ofs(tmp_file); - ofs << content; - ofs.close(); - } - - // Read through the C++ wrapper which calls DP_ReadFileToChar2 - std::string file_content; - deepmd::read_file_to_string(tmp_file, file_content); - - // The full content must be preserved byte-for-byte. - EXPECT_EQ(content.size(), file_content.size()) - << "Size mismatch: the file has " << content.size() - << " bytes but read_file_to_string returned " << file_content.size(); - EXPECT_EQ(content, file_content) - << "Content mismatch: trailing whitespace was not preserved"; - - // Clean up the temporary file. - std::remove(tmp_file.c_str()); -} diff --git a/source/tests/test_c_api_readfile.py b/source/tests/test_c_api_readfile.py deleted file mode 100644 index 3d2b7b32b7..0000000000 --- a/source/tests/test_c_api_readfile.py +++ /dev/null @@ -1,103 +0,0 @@ -# SPDX-License-Identifier: LGPL-3.0-or-later -"""Regression test for ``DP_ReadFileToChar2`` / ``read_file_to_string``. - -See https://github.com/deepmodeling/deepmd-kit/issues/5620 for context. -Previously ``DP_ReadFileToChar2`` reported the original file size but -returned a buffer produced by ``string_to_char``, which trims trailing -whitespace before allocating/copying. The C++ wrapper then reconstructed a -``std::string`` with the reported (larger) size, causing an over-read of the -shorter allocation. This test verifies that exact bytes are preserved, -including trailing whitespace. -""" - -import ctypes -import pathlib - -import pytest - - -def _load_c_lib(): - """Load the DeePMD C library (``libdeepmd_c.so``). - - The library ships with the Python package; we search the standard - locations and fall back to the build tree. - """ - import deepmd - - candidates = [ - pathlib.Path(deepmd.__file__).parent / "lib" / "libdeepmd_c.so", - pathlib.Path(__file__).resolve().parents[2] / "dp" / "lib" / "libdeepmd_c.so", - ] - for path in candidates: - if path.exists(): - return ctypes.CDLL(str(path)) - pytest.skip("libdeepmd_c.so not found") - return None - - -@pytest.fixture(scope="module") -def c_lib(): - """Module-scoped fixture that loads the C library and configures - the DP_ReadFileToChar2 / DP_DeleteChar signatures once. - """ - lib = _load_c_lib() - lib.DP_ReadFileToChar2.argtypes = [ - ctypes.c_char_p, - ctypes.POINTER(ctypes.c_int), - ] - lib.DP_ReadFileToChar2.restype = ctypes.c_void_p - lib.DP_DeleteChar.argtypes = [ctypes.c_void_p] - lib.DP_DeleteChar.restype = None - return lib - - -def test_readfiletochar2_preserves_trailing_whitespace(tmp_path, c_lib): - """Files ending in whitespace must return exact size and bytes.""" - # Write a temporary file whose content ends with " \n" (trailing - # whitespace). This is the exact scenario that was broken: the old - # code trimmed the whitespace but reported the original size. - content = b"hello world \n" # ends with space + newline - tmp_file = tmp_path / "test_exact_bytes.txt" - tmp_file.write_bytes(content) - - # Call DP_ReadFileToChar2 ---------------------------------------------- - # Use c_void_p for the return type so ctypes does not truncate the - # buffer at the first null byte (c_char_p would do that). - size = ctypes.c_int(0) - c_buf = c_lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) - assert c_buf is not None, "DP_ReadFileToChar2 returned NULL" - - size_val = size.value - # Negative size indicates an error (the error message is in the buffer). - assert size_val >= 0, ( - f"DP_ReadFileToChar2 error: {ctypes.string_at(c_buf, -size_val)}" - ) - - # The reported size must match the file size exactly. - assert size_val == len(content), ( - f"Size mismatch: file has {len(content)} bytes but " - f"DP_ReadFileToChar2 reported {size_val}" - ) - - # The returned buffer must contain the exact file bytes, including - # trailing whitespace. - returned_bytes = ctypes.string_at(c_buf, size_val) - assert returned_bytes == content, ( - f"Content mismatch: expected {content!r} but got {returned_bytes!r}" - ) - - # Clean up the allocated buffer. - c_lib.DP_DeleteChar(c_buf) - - -def test_readfiletochar2_preserves_no_trailing_whitespace(tmp_path, c_lib): - """Sanity check: files without trailing whitespace still work.""" - content = b"hello world" - tmp_file = tmp_path / "test_no_trailing.txt" - tmp_file.write_bytes(content) - - size = ctypes.c_int(0) - c_buf = c_lib.DP_ReadFileToChar2(str(tmp_file).encode("utf-8"), ctypes.byref(size)) - assert size.value == len(content) - assert ctypes.string_at(c_buf, size.value) == content - c_lib.DP_DeleteChar(c_buf)