fix: guard Binstream write path against unopened files and failed fwrite - #8025
Merged
Merged
Conversation
Fixes deepmodeling#7562. The binary wavefunction writers (wfc_nao_write2file and wfc_nao_write2file_complex) used to only print a warning when the output file could not be opened, then kept writing through a null FILE pointer, which could crash or produce corrupt files. Binstream::operator<< and Binstream::write now check that the stream is open and verify the fwrite return value, matching the existing read-path behavior. The call sites in write_wfc_nao.cpp now terminate with WARNING_QUIT on open failure instead of continuing. Tests added: - BinstreamTest.WriteToUnopenedFile - ModuleIOTest.WriteWfcNaoBinaryOpenFail - ModuleIOTest.WriteWfcNaoComplexBinaryOpenFail
AsTonyshment
left a comment
Collaborator
There was a problem hiding this comment.
Some small suggestions.
…through WARNING_QUIT fwrite() reports success while data is still buffered, and both close() and the destructor used to ignore fclose() failures, so a delayed write error (e.g. RLIMIT_FSIZE, full disk) could be silently dropped: with RLIMIT_FSIZE=1 the NAO binary writers returned normally with only one byte on disk. Changes: - operator<< and write() now fflush() after fwrite() so buffered write errors surface at the call site; the array overload had the same gap. - close() checks the fclose() result and WARNING_QUITs on failure, and is now a safe no-op on an unopened stream (fclose(NULL) was UB). - ~Binstream() checks fclose() but only WARNINGs, since a destructor must not terminate the program. - All error paths (read/write, scalar/array) now exit via ModuleBase::WARNING_QUIT (exit code 1, warning.log, MPI-safe) instead of std::cout + exit(0/1). - operator>> and read() now reject an unopened stream instead of calling fread(NULL), matching the write path. - open() closes any previously opened file first instead of leaking the old handle; copy/assignment are deleted (FILE* ownership would double-close). - Error messages: fix "didn't be" grammar and the misleading "dynamic memory" wording for array overloads. Tests added/updated: - BinstreamTest.DelayedWriteFailureDetected: RLIMIT_FSIZE=1 + ignored SIGXFSZ, asserts write() exits with code 1. - BinstreamTest.ReadFromUnopenedFile, BinstreamTest.CloseFailureDetected. - Existing death tests updated to ExitedWithCode(1) and the "!NOTICE!" marker. No docs update needed: no INPUT parameter or user-facing interface behavior changed, only failure handling of binary I/O. Verified: make MODULE_BASE_binstream && ctest -R MODULE_BASE_binstream (6/6 passed); full make -j8 with no errors.
AsTonyshment
approved these changes
Sep 26, 2026
7 tasks done
Critsium-xy
added a commit
to Critsium-xy/abacus-develop
that referenced
this pull request
Sep 28, 2026
.editorconfig has required `indent_style = space` with `indent_size = 4` for the whole repository, but 339 files under source/ still indent with tabs. This converts the leading whitespace of 248 of them. 253 files were selected when this was prepared. Since then, develop has deleted source_lcao/setup_dm.h (deepmodeling#8000) and source_pw/module_stodft/hamilt_sdft_pw.cpp (deepmodeling#8012), and already converted source_base/module_out/binstream.{h,cpp} (deepmodeling#8025) and source_pw/module_stodft/sto_hamilt_pw.h (renamed from hamilt_sdft_pw.h in deepmodeling#8012), which leaves 248. The conversion follows files that develop moved, e.g. onsite_proj_tools_stress.cpp is now under module_proj/ (deepmodeling#8007). Scope. Three groups were deliberately left out: - 48 files touched by the pull requests open at the time of writing (deepmodeling#8000, deepmodeling#7924, deepmodeling#7906, deepmodeling#8005 and others), so this does not force a conflict on work in flight; - 5 vendored files: source_base/libm/ is ported from glibc-2.36 and carries its own LICENCE, and source_base/mcd.c is Softpixel MemCheckDeluxe under a BSD licence. Reformatting vendored sources makes future syncs with their upstream harder; - tabs that appear after the first non-blank character (alignment tabs, 1193 lines). Only leading indentation is converted. Method and verification. `expand -i -t4`, which rewrites the initial whitespace of a line and nothing else, followed by three checks: - `git diff -w --stat` is empty, so not one non-whitespace character changed anywhere in the diff; - of the 42 raw string literals in the changed files (222 lines, all in source_io/module_parameter/read_inp_out.cpp), none has a line that this commit touches. Leading whitespace inside `R"(...)"` is part of the string, so that was the one place a leading-whitespace rewrite could have changed behaviour; - no changed line follows a line ending in a backslash, so no backslash-continued string literal is affected either; - no file gained a CR. Effect on tools/03_code_analysis/code_quality_score.py: average score over source/ goes from 82.15 to 82.58 and the number of passing files from 1560 to 1562. Six files score 1 to 3 points lower, all through the `line_too_long` rule, because the scorer counts a tab as a single character while it renders as up to four columns. The lines were already over 120 columns on screen; the tab was hiding it. One of them, module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just below the tool's pass line. Wrapping those lines would mean editing code in a commit that is otherwise whitespace-only, so it is left for a follow-up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Critsium-xy
added a commit
to Critsium-xy/abacus-develop
that referenced
this pull request
Sep 30, 2026
.editorconfig has required `indent_style = space` with `indent_size = 4` for the whole repository, but 339 files under source/ still indent with tabs. This converts the leading whitespace of 246 of them. 253 files were selected when this was prepared. Since then, develop has deleted two of them and already converted five others, which leaves 246: - deleted: source_lcao/setup_dm.h (deepmodeling#8000), source_pw/module_stodft/hamilt_sdft_pw.cpp (deepmodeling#8012); - already converted: source_base/module_out/binstream.{h,cpp} (deepmodeling#8025), source_pw/module_stodft/sto_hamilt_pw.h (renamed from hamilt_sdft_pw.h in deepmodeling#8012), source_io/module_ctrl/ctrl_output_pw.h and source_pw/module_pwdft/op_pw_nl.cpp (deepmodeling#8043). Files that develop moved are converted at their new path, e.g. onsite_proj_tools_stress.cpp is now under source_pw/module_proj/ (deepmodeling#8007). Scope. Three groups were deliberately left out: - 48 files touched by the pull requests open at the time of writing (deepmodeling#8000, deepmodeling#7924, deepmodeling#7906, deepmodeling#8005 and others), so this does not force a conflict on work in flight; - 5 vendored files: source_base/libm/ is ported from glibc-2.36 and carries its own LICENCE, and source_base/mcd.c is Softpixel MemCheckDeluxe under a BSD licence. Reformatting vendored sources makes future syncs with their upstream harder; - tabs that appear after the first non-blank character (alignment tabs, 1193 lines). Only leading indentation is converted. Method and verification. `expand -i -t4`, which rewrites the initial whitespace of a line and nothing else, followed by three checks: - `git diff -w --stat` is empty, so not one non-whitespace character changed anywhere in the diff; - of the 42 raw string literals in the changed files (222 lines, all in source_io/module_parameter/read_inp_out.cpp), none has a line that this commit touches. Leading whitespace inside `R"(...)"` is part of the string, so that was the one place a leading-whitespace rewrite could have changed behaviour; - no changed line follows a line ending in a backslash, so no backslash-continued string literal is affected either; - no file gained a CR. Effect on tools/03_code_analysis/code_quality_score.py: average score over source/ goes from 82.15 to 82.58 and the number of passing files from 1560 to 1562. Six files score 1 to 3 points lower, all through the `line_too_long` rule, because the scorer counts a tab as a single character while it renders as up to four columns. The lines were already over 120 columns on screen; the tab was hiding it. One of them, module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just below the tool's pass line. Wrapping those lines would mean editing code in a commit that is otherwise whitespace-only, so it is left for a follow-up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
mohanchen
pushed a commit
that referenced
this pull request
Sep 30, 2026
…o spaces (#8011) * docs: fix two @file tags that name a different file Doxygen's @file takes the name of the file it documents. These two name a file that does not exist, so Doxygen attributes the block to the wrong (or to no) file: source_base/ndarray.h said NDArray.h source_lcao/module_rt/band_energy.h said bandenegy.h (also a typo) Found by tools/03_code_analysis/code_quality_score.py (rule doc_file_mismatch). The scan also flagged source_pw/module_pwdft/radial_proj.h (said radial_projection.h), but #8007 has since moved that header to source_pw/module_proj/ with the tag already corrected, so it is no longer part of this commit. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * style: convert leading tab indentation to spaces in 246 source files .editorconfig has required `indent_style = space` with `indent_size = 4` for the whole repository, but 339 files under source/ still indent with tabs. This converts the leading whitespace of 246 of them. 253 files were selected when this was prepared. Since then, develop has deleted two of them and already converted five others, which leaves 246: - deleted: source_lcao/setup_dm.h (#8000), source_pw/module_stodft/hamilt_sdft_pw.cpp (#8012); - already converted: source_base/module_out/binstream.{h,cpp} (#8025), source_pw/module_stodft/sto_hamilt_pw.h (renamed from hamilt_sdft_pw.h in #8012), source_io/module_ctrl/ctrl_output_pw.h and source_pw/module_pwdft/op_pw_nl.cpp (#8043). Files that develop moved are converted at their new path, e.g. onsite_proj_tools_stress.cpp is now under source_pw/module_proj/ (#8007). Scope. Three groups were deliberately left out: - 48 files touched by the pull requests open at the time of writing (#8000, #7924, #7906, #8005 and others), so this does not force a conflict on work in flight; - 5 vendored files: source_base/libm/ is ported from glibc-2.36 and carries its own LICENCE, and source_base/mcd.c is Softpixel MemCheckDeluxe under a BSD licence. Reformatting vendored sources makes future syncs with their upstream harder; - tabs that appear after the first non-blank character (alignment tabs, 1193 lines). Only leading indentation is converted. Method and verification. `expand -i -t4`, which rewrites the initial whitespace of a line and nothing else, followed by three checks: - `git diff -w --stat` is empty, so not one non-whitespace character changed anywhere in the diff; - of the 42 raw string literals in the changed files (222 lines, all in source_io/module_parameter/read_inp_out.cpp), none has a line that this commit touches. Leading whitespace inside `R"(...)"` is part of the string, so that was the one place a leading-whitespace rewrite could have changed behaviour; - no changed line follows a line ending in a backslash, so no backslash-continued string literal is affected either; - no file gained a CR. Effect on tools/03_code_analysis/code_quality_score.py: average score over source/ goes from 82.15 to 82.58 and the number of passing files from 1560 to 1562. Six files score 1 to 3 points lower, all through the `line_too_long` rule, because the scorer counts a tab as a single character while it renders as up to four columns. The lines were already over 120 columns on screen; the tab was hiding it. One of them, module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just below the tool's pass line. Wrapping those lines would mean editing code in a commit that is otherwise whitespace-only, so it is left for a follow-up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7562.
The binary wavefunction writers (wfc_nao_write2file and wfc_nao_write2file_complex) used to only print a warning when the output file could not be opened, then kept writing through a null FILE pointer, which could crash or produce corrupt files.
Binstream::operator<< and Binstream::write now check that the stream is open and verify the fwrite return value, matching the existing read-path behavior. The call sites in write_wfc_nao.cpp now terminate with WARNING_QUIT on open failure instead of continuing.
Tests added:
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.source/changes.Linked Issue
Fix #
Unit Tests and/or Case Tests for my changes
What's changed?
Governance Notes