Skip to content

Commit 0601c79

Browse files
authored
fix(api_c): preserve exact bytes in DP_ReadFileToChar2 (deepmodeling#5763)
## Fix deepmodeling#5620 ### Problem `DP_ReadFileToChar气Char2` reported the original file size but returned a buffer from `string_to_char`, which trims trailing whitespace before allocating/copying. The C++ wrapper (`read_file_to_string` in `deepmd.hpp`) then reconstructed a `std::string` with the reported (larger) size, causing an over-read of the shorter allocation. ### Fix Added `string_to_char_exact`, a new 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. ### Test 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. The Python test uses `ctypes` to call `DP_ReadFileToChar2` directly and the C API. Verified: the test fails against the buggy code (over-reads garbage bytes) and passes with the fix. ### Attribution Generated with [opencode](https://opencode.ai) using model `glm-5.2`. ### Recommended reviewers @njzjz (maintainer of the C API) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added per-frame `charge_spin` support to DeepPot and model-deviation C API computations. - Introduced “version 3” compute entry points that accept `charge_spin` (including neighbor-list variants). - Added APIs to query the required charge-spin dimension. - **Bug Fixes** - File-reading C APIs now preserve exact bytes, including trailing whitespace/newlines, and add safer size handling. - **Tests** - Added regression tests (C++ and Python) validating byte-for-byte file content preservation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: njzjz-bot <njzjz.bot@gmail.com>
1 parent 716521f commit 0601c79

3 files changed

Lines changed: 120 additions & 39 deletions

File tree

source/api_c/include/c_api.h

Lines changed: 40 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -682,25 +682,26 @@ extern void DP_DeepPotComputeNListf2(DP_DeepPot* dp,
682682
* @param[in] dp The DP to use.
683683
* @param[in] nframes The number of frames.
684684
* @param[in] natoms The number of atoms.
685-
* @param[in] coord The coordinates of atoms. The array should be of size natoms
686-
*x 3.
685+
* @param[in] coord The coordinates of atoms. The array should be of size
686+
*nframes x natoms x 3.
687687
* @param[in] atype The atom types. The array should contain natoms ints.
688-
* @param[in] cell The cell of the region. The array should be of size 9. Pass
689-
*NULL if pbc is not used.
688+
* @param[in] cell The cell of the region. The array should be of size nframes x
689+
*9. Pass NULL if pbc is not used.
690690
* @param[in] fparam The frame parameters. The array can be of size nframes x
691691
*dim_fparam.
692692
* @param[in] aparam The atom parameters. The array can be of size nframes x
693693
*natoms x dim_aparam.
694694
* @param[in] charge_spin The per-frame charge/spin input. The array can be of
695695
*size nframes x dim_chg_spin. Pass NULL to use the model's stored
696696
*default_chg_spin.
697-
* @param[out] energy Output energy.
698-
* @param[out] force Output force. The array should be of size natoms x 3.
699-
* @param[out] virial Output virial. The array should be of size 9.
697+
* @param[out] energy Output energy. The array should be of size nframes.
698+
* @param[out] force Output force. The array should be of size nframes x natoms
699+
*x 3.
700+
* @param[out] virial Output virial. The array should be of size nframes x 9.
700701
* @param[out] atomic_energy Output atomic energy. The array should be of size
701-
*natoms.
702+
*nframes x natoms.
702703
* @param[out] atomic_virial Output atomic virial. The array should be of size
703-
*natoms x 9.
704+
*nframes x natoms x 9.
704705
* @warning The output arrays should be allocated before calling this function.
705706
*Pass NULL if not required.
706707
* @since API version 27
@@ -727,25 +728,26 @@ extern void DP_DeepPotCompute3(DP_DeepPot* dp,
727728
* @param[in] dp The DP to use.
728729
* @param[in] nframes The number of frames.
729730
* @param[in] natoms The number of atoms.
730-
* @param[in] coord The coordinates of atoms. The array should be of size natoms
731-
*x 3.
731+
* @param[in] coord The coordinates of atoms. The array should be of size
732+
*nframes x natoms x 3.
732733
* @param[in] atype The atom types. The array should contain natoms ints.
733-
* @param[in] cell The cell of the region. The array should be of size 9. Pass
734-
*NULL if pbc is not used.
734+
* @param[in] cell The cell of the region. The array should be of size nframes x
735+
*9. Pass NULL if pbc is not used.
735736
* @param[in] fparam The frame parameters. The array can be of size nframes x
736737
*dim_fparam.
737738
* @param[in] aparam The atom parameters. The array can be of size nframes x
738739
*natoms x dim_aparam.
739740
* @param[in] charge_spin The per-frame charge/spin input. The array can be of
740741
*size nframes x dim_chg_spin. Pass NULL to use the model's stored
741742
*default_chg_spin.
742-
* @param[out] energy Output energy.
743-
* @param[out] force Output force. The array should be of size natoms x 3.
744-
* @param[out] virial Output virial. The array should be of size 9.
743+
* @param[out] energy Output energy. The array should be of size nframes.
744+
* @param[out] force Output force. The array should be of size nframes x natoms
745+
*x 3.
746+
* @param[out] virial Output virial. The array should be of size nframes x 9.
745747
* @param[out] atomic_energy Output atomic energy. The array should be of size
746-
*natoms.
748+
*nframes x natoms.
747749
* @param[out] atomic_virial Output atomic virial. The array should be of size
748-
*natoms x 9.
750+
*nframes x natoms x 9.
749751
* @warning The output arrays should be allocated before calling this function.
750752
*Pass NULL if not required.
751753
* @since API version 27
@@ -772,11 +774,11 @@ extern void DP_DeepPotComputef3(DP_DeepPot* dp,
772774
* @param[in] dp The DP to use.
773775
* @param[in] nframes The number of frames.
774776
* @param[in] natoms The number of atoms.
775-
* @param[in] coord The coordinates of atoms. The array should be of size natoms
776-
*x 3.
777+
* @param[in] coord The coordinates of atoms. The array should be of size
778+
*nframes x natoms x 3.
777779
* @param[in] atype The atom types. The array should contain natoms ints.
778-
* @param[in] cell The cell of the region. The array should be of size 9. Pass
779-
*NULL if pbc is not used.
780+
* @param[in] cell The cell of the region. The array should be of size nframes x
781+
*9. Pass NULL if pbc is not used.
780782
* @param[in] nghost The number of ghost atoms.
781783
* @param[in] nlist The neighbor list.
782784
* @param[in] ago Update the internal neighbour list if ago is 0.
@@ -787,13 +789,14 @@ extern void DP_DeepPotComputef3(DP_DeepPot* dp,
787789
* @param[in] charge_spin The per-frame charge/spin input. The array can be of
788790
*size nframes x dim_chg_spin. Pass NULL to use the model's stored
789791
*default_chg_spin.
790-
* @param[out] energy Output energy.
791-
* @param[out] force Output force. The array should be of size natoms x 3.
792-
* @param[out] virial Output virial. The array should be of size 9.
792+
* @param[out] energy Output energy. The array should be of size nframes.
793+
* @param[out] force Output force. The array should be of size nframes x natoms
794+
*x 3.
795+
* @param[out] virial Output virial. The array should be of size nframes x 9.
793796
* @param[out] atomic_energy Output atomic energy. The array should be of size
794-
*natoms.
797+
*nframes x natoms.
795798
* @param[out] atomic_virial Output atomic virial. The array should be of size
796-
*natoms x 9.
799+
*nframes x natoms x 9.
797800
* @warning The output arrays should be allocated before calling this function.
798801
*Pass NULL if not required.
799802
* @since API version 27
@@ -823,11 +826,11 @@ extern void DP_DeepPotComputeNList3(DP_DeepPot* dp,
823826
* @param[in] dp The DP to use.
824827
* @param[in] nframes The number of frames.
825828
* @param[in] natoms The number of atoms.
826-
* @param[in] coord The coordinates of atoms. The array should be of size natoms
827-
*x 3.
829+
* @param[in] coord The coordinates of atoms. The array should be of size
830+
*nframes x natoms x 3.
828831
* @param[in] atype The atom types. The array should contain natoms ints.
829-
* @param[in] cell The cell of the region. The array should be of size 9. Pass
830-
*NULL if pbc is not used.
832+
* @param[in] cell The cell of the region. The array should be of size nframes x
833+
*9. Pass NULL if pbc is not used.
831834
* @param[in] nghost The number of ghost atoms.
832835
* @param[in] nlist The neighbor list.
833836
* @param[in] ago Update the internal neighbour list if ago is 0.
@@ -838,13 +841,14 @@ extern void DP_DeepPotComputeNList3(DP_DeepPot* dp,
838841
* @param[in] charge_spin The per-frame charge/spin input. The array can be of
839842
*size nframes x dim_chg_spin. Pass NULL to use the model's stored
840843
*default_chg_spin.
841-
* @param[out] energy Output energy.
842-
* @param[out] force Output force. The array should be of size natoms x 3.
843-
* @param[out] virial Output virial. The array should be of size 9.
844+
* @param[out] energy Output energy. The array should be of size nframes.
845+
* @param[out] force Output force. The array should be of size nframes x natoms
846+
*x 3.
847+
* @param[out] virial Output virial. The array should be of size nframes x 9.
844848
* @param[out] atomic_energy Output atomic energy. The array should be of size
845-
*natoms.
849+
*nframes x natoms.
846850
* @param[out] atomic_virial Output atomic virial. The array should be of size
847-
*natoms x 9.
851+
*nframes x natoms x 9.
848852
* @warning The output arrays should be allocated before calling this function.
849853
*Pass NULL if not required.
850854
* @since API version 27

source/api_c/src/c_api.cc

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// SPDX-License-Identifier: LGPL-3.0-or-later
22
#include "c_api.h"
33

4+
#include <limits>
45
#include <numeric>
56
#include <string>
67
#include <vector>
@@ -1535,6 +1536,27 @@ const char* string_to_char(std::string& str) {
15351536
return buffer;
15361537
}
15371538

1539+
/**
1540+
* @brief Convert std::string to const char without trimming.
1541+
*
1542+
* Unlike string_to_char, this helper preserves every byte of the input,
1543+
* including trailing whitespace. This is necessary for file-reading
1544+
* functions (e.g. DP_ReadFileToChar2) where the reported size must
1545+
* match the allocated buffer exactly; see issue #5620.
1546+
*
1547+
* @param[in] str std::string to be converted (not modified)
1548+
* @return const char* heap-allocated buffer, caller must DP_DeleteChar
1549+
*/
1550+
const char* string_to_char_exact(const std::string& str) {
1551+
// copy from string to char* without any trimming
1552+
const std::string::size_type size = str.size();
1553+
// +1 for '\0'
1554+
char* buffer = new char[size + 1];
1555+
std::copy(str.begin(), str.end(), buffer);
1556+
buffer[size] = '\0';
1557+
return buffer;
1558+
}
1559+
15381560
extern "C" {
15391561

15401562
const char* DP_NlistCheckOK(DP_Nlist* nlist) {
@@ -2642,7 +2664,8 @@ const char* DP_ReadFileToChar(const char* c_model) {
26422664
std::string model(c_model);
26432665
std::string file_content;
26442666
deepmd::read_file_to_string(model, file_content);
2645-
return string_to_char(file_content);
2667+
// Preserve exact bytes — see issue #5620 for why trimming is wrong here.
2668+
return string_to_char_exact(file_content);
26462669
}
26472670

26482671
const char* DP_ReadFileToChar2(const char* c_model, int* size) {
@@ -2656,8 +2679,20 @@ const char* DP_ReadFileToChar2(const char* c_model, int* size) {
26562679
*size = -error_message.size();
26572680
return string_to_char(error_message);
26582681
}
2659-
*size = file_content.size();
2660-
return string_to_char(file_content);
2682+
// Record the exact file size before any conversion. We must use
2683+
// string_to_char_exact (not string_to_char) so that trailing
2684+
// whitespace is preserved and the returned buffer has exactly *size
2685+
// bytes — otherwise the C++ wrapper would reconstruct a string that
2686+
// over-reads the shorter allocation. See issue #5620.
2687+
if (file_content.size() >
2688+
static_cast<std::size_t>(std::numeric_limits<int>::max())) {
2689+
std::string error_message =
2690+
"File is too large to be read into a char buffer via this API";
2691+
*size = -static_cast<int>(error_message.size());
2692+
return string_to_char(error_message);
2693+
}
2694+
*size = static_cast<int>(file_content.size());
2695+
return string_to_char_exact(file_content);
26612696
}
26622697

26632698
void DP_SelectByType(const int natoms,

source/api_c/tests/test_read_file_to_string.cc

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,14 @@
66

77
#include <algorithm>
88
#include <cmath>
9+
#include <cstdio>
910
#include <fstream>
11+
#include <memory>
1012
#include <sstream>
1113
#include <string>
1214
#include <vector>
1315

16+
#include "c_api.h"
1417
#include "deepmd.hpp"
1518
TEST(TestReadFileToString, readfiletostring) {
1619
#ifndef BUILD_TENSORFLOW
@@ -40,3 +43,42 @@ TEST(TestReadFileToString, readfiletostringerr) {
4043
},
4144
deepmd::hpp::deepmd_exception);
4245
}
46+
47+
TEST(TestReadFileToString, c_api_preserves_exact_bytes) {
48+
const std::string file_name = "test_read_file_to_char_exact_bytes.bin";
49+
const std::string expected("hello\0world \n", 13);
50+
{
51+
std::ofstream output(file_name, std::ios::binary);
52+
ASSERT_TRUE(output.is_open());
53+
output.write(expected.data(), expected.size());
54+
}
55+
56+
int size = 0;
57+
std::unique_ptr<const char, decltype(&DP_DeleteChar)> content(
58+
DP_ReadFileToChar2(file_name.c_str(), &size), DP_DeleteChar);
59+
60+
ASSERT_NE(content, nullptr);
61+
ASSERT_GE(size, 0);
62+
EXPECT_EQ(size, expected.size());
63+
EXPECT_EQ(std::string(content.get(), size), expected);
64+
65+
EXPECT_EQ(std::remove(file_name.c_str()), 0);
66+
}
67+
68+
TEST(TestReadFileToString, legacy_c_api_preserves_trailing_whitespace) {
69+
const std::string file_name = "test_read_file_to_char_whitespace.txt";
70+
const std::string expected = "hello world \n";
71+
{
72+
std::ofstream output(file_name, std::ios::binary);
73+
ASSERT_TRUE(output.is_open());
74+
output.write(expected.data(), expected.size());
75+
}
76+
77+
std::unique_ptr<const char, decltype(&DP_DeleteChar)> content(
78+
DP_ReadFileToChar(file_name.c_str()), DP_DeleteChar);
79+
80+
ASSERT_NE(content, nullptr);
81+
EXPECT_EQ(std::string(content.get()), expected);
82+
83+
EXPECT_EQ(std::remove(file_name.c_str()), 0);
84+
}

0 commit comments

Comments
 (0)