[WIP] Follow-up to #5633: trim filesystem::read buffer to gcount() for text-mode streams#5634
Closed
wnykuang wants to merge 1 commit into
Closed
[WIP] Follow-up to #5633: trim filesystem::read buffer to gcount() for text-mode streams#5634wnykuang wants to merge 1 commit into
wnykuang wants to merge 1 commit into
Conversation
On Windows, ifstreams opened in text mode translate CRLF to LF, so istream::read() delivers fewer characters than the on-disk length computed by seeking to the end. The buffer was sized to the on-disk length and never trimmed, leaving a tail of NUL bytes. This made read_as_string(path) return corrupted content for any CRLF file on Windows, which broke the Filetypes.EpwFile_UTF8BOM test (*.epw resources are checked out with CRLF): the NUL tail parsed as a garbage 8769th line, failing with 'Insufficient weather data'. Trim the buffer to gcount() after reading. Binary-mode reads are unaffected since the character count always matches the byte length.
Contributor
Author
|
will double verify the code change in the windows x64; will send doc too. |
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
1 participant
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.
Pull request overview
Follow-up hardening for #5629 / #5633 — this closes the remaining gap from the same root cause fixed in those PRs.
#5633 fixed
read_as_string(const path&)by opening the file in binary mode. However, the underlyingread(openstudio::filesystem::ifstream&)helper is still vulnerable when a caller-supplied stream was opened in text mode: it sizes the buffer from the on-disk byte count (seekg/tellg), but on Windows the text-mode CRLF→LF translation makesistream::read()deliver fewer characters than that, leaving a tail of zero-initialized (NUL) bytes in the returned buffer — silently.There is at least one live path hitting this today:
PrjModelImpl.cpp:78opensifstream file(path)(text mode) and hands it tocontam::Reader→read_as_string(ifstream&)→read(ifstream&). On Windows, a CONTAM.prjfile with CRLF line endings gets the same NUL-padded corruption that brokeFiletypes.EpwFile_UTF8BOM.Fix: after
read(), trim the buffer togcount()— the number of characters actually delivered. Binary-mode reads are unaffected (character count always equals the byte length), so this is a no-op on POSIX and for all existing binary callers.Verified on Linux by rebuilding
openstudio_utilities_tests: the full-suite failure list is byte-identical before/after the change (only pre-existing local environment failures), and allFiletypes.EpwFile*tests pass.Note: I will follow up on this PR with the related documentation updates required for this project before it is ready for full review.
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PR