This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix incorrect length check in std::basic_filebuf
ClosedPublic

Authored by ldionne on Jul 5 2023, 8:28 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG574c5cc9d8f6: [libc++] Fix incorrect length check in std::basic_filebuf
Summary

This patch fixes an ASAN-found issue in std::basic_filebuf where we'd
check the wrong size before proceeding to set our internal buffer to
the externally-provided buffer, leading to the library trying to read
from the incorrect buffer in underflow().

Thanks to Andrey Semin for the patch.

Diff Detail

Event Timeline

ldionne created this revision.Jul 5 2023, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 8:28 AM
ldionne requested review of this revision.Jul 5 2023, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 8:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@mstorsjo I am seeing some errors on Windows, and on Windows only: https://buildkite.com/llvm-project/libcxx-ci/builds/27632.

Do you think you (or someone else seeing this w/ Windows access) can help troubleshoot this? I expect this is going to be challenging to troubleshoot without being able to repro locally. Otherwise, I'll XFAIL the two tests I added on Windows.

@mstorsjo I am seeing some errors on Windows, and on Windows only: https://buildkite.com/llvm-project/libcxx-ci/builds/27632.

Do you think you (or someone else seeing this w/ Windows access) can help troubleshoot this? I expect this is going to be challenging to troubleshoot without being able to repro locally. Otherwise, I'll XFAIL the two tests I added on Windows.

I can have a look, but probably only within a couple days.

@mstorsjo I am seeing some errors on Windows, and on Windows only: https://buildkite.com/llvm-project/libcxx-ci/builds/27632.

Do you think you (or someone else seeing this w/ Windows access) can help troubleshoot this? I expect this is going to be challenging to troubleshoot without being able to repro locally. Otherwise, I'll XFAIL the two tests I added on Windows.

By default the streams are opened in text mode, and in text mode, if you output \n, it will actually write \r\n to the file. By opening all streams in binary mode, you bypass such translations; add std::ios::binary to the open() calls. With the attached suggested edits, the buffered_reads test seems to pass for me, I presume the corresponding change for the buffered_writes test fixes that too.

libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/buffered_reads.pass.cpp
56
72
112
129
ldionne marked 4 inline comments as done.Oct 26 2023, 7:07 AM

By default the streams are opened in text mode, and in text mode, if you output \n, it will actually write \r\n to the file. By opening all streams in binary mode, you bypass such translations; add std::ios::binary to the open() calls. With the attached suggested edits, the buffered_reads test seems to pass for me, I presume the corresponding change for the buffered_writes test fixes that too.

That's awesome, thanks so much for investigating! I'm updating the patch right now, hopefully CI should pass now.

ldionne updated this revision to Diff 557896.Oct 26 2023, 7:08 AM

Fix Windows tests.

ldionne accepted this revision.Oct 27 2023, 7:21 AM

The only CI failure is a flaky test on FreeBSD, so I think this is good to go. I'll merge this now since there hasn't been much discussion and I think it shouldn't be controversial to fix an ASAN issue w/ tests.

This revision is now accepted and ready to land.Oct 27 2023, 7:21 AM
This revision was landed with ongoing or failed builds.Oct 27 2023, 7:22 AM
This revision was automatically updated to reflect the committed changes.