This is an archive of the discontinued LLVM Phabricator instance.

Differential D139878

[libc] remove use after free in tests.
ClosedPublic

Authored by michaelrj on Dec 12 2022, 1:22 PM.

Details

Reviewers
None
Commits
rG816b5e546c65: [libc] remove use after free in tests.
Summary

There were some tests added that attempted to access files after they
were closed to test errno behavior, this caused sanitizer issues. Those
portions of the tests have been disabled.

Diff Detail

Event Timeline

michaelrj created this revision.Dec 12 2022, 1:22 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 12 2022, 1:22 PM
Herald added subscribers: libc-commits, ecnelises, tschuett. · View Herald Transcript
michaelrj requested review of this revision.Dec 12 2022, 1:22 PM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 12 2022, 1:23 PM
This revision was landed with ongoing or failed builds.
Closed by commit rG816b5e546c65: [libc] remove use after free in tests. (authored by michaelrj). · Explain Why
This revision was automatically updated to reflect the committed changes.
michaelrj added a commit: rG816b5e546c65: [libc] remove use after free in tests..
Harbormaster completed remote builds in B202673: Diff 482246.Dec 12 2022, 1:29 PM
sivachandra added a subscriber: sivachandra.Dec 12 2022, 1:32 PM
sivachandra added inline comments.
libc/test/src/stdio/fileop_test.cpp
128

Looks like these were added in https://reviews.llvm.org/D139576. If there are incorrect, they should be removed. If they are correct and the fix is expected to land separately, then please file a bug explaining the expected behavior in detail and add a TODO here which links to that bug.

Revision Contents

PathSize
libc/
test/
src/
stdio/
24 lines
6 lines

Diff 482247

libc/test/src/stdio/fileop_test.cpp

Show First 20 Lines • Show All 108 Lines • ▼ Show 20 LinesTEST(LlvmLibcFILETest, SimpleFileOperations) {
ASSERT_EQ(__llvm_libc::fread(read_data, 1, sizeof(CONTENT) - 1, file), ASSERT_EQ(__llvm_libc::fread(read_data, 1, sizeof(CONTENT) - 1, file),
sizeof(CONTENT) - 1); sizeof(CONTENT) - 1);
read_data[sizeof(CONTENT) - 1] = '\0'; read_data[sizeof(CONTENT) - 1] = '\0';
ASSERT_STREQ(read_data, CONTENT); ASSERT_STREQ(read_data, CONTENT);
ASSERT_EQ(__llvm_libc::fclose(file), 0); ASSERT_EQ(__llvm_libc::fclose(file), 0);
// Check that the other functions correctly set errno. // Check that the other functions correctly set errno.
errno = 0; // errno = 0;
ASSERT_NE(__llvm_libc::fseek(file, 0, SEEK_SET), 0); // ASSERT_NE(__llvm_libc::fseek(file, 0, SEEK_SET), 0);
EXPECT_NE(errno, 0); // EXPECT_NE(errno, 0);
errno = 0; // errno = 0;
ASSERT_NE(__llvm_libc::fclose(file), 0); // ASSERT_NE(__llvm_libc::fclose(file), 0);
EXPECT_NE(errno, 0); // EXPECT_NE(errno, 0);
errno = 0; // errno = 0;
ASSERT_EQ(__llvm_libc::fopen("INVALID FILE NAME", "r"), // ASSERT_EQ(__llvm_libc::fopen("INVALID FILE NAME", "r"),
static_cast<FILE *>(nullptr)); // static_cast<FILE *>(nullptr));
EXPECT_NE(errno, 0); // EXPECT_NE(errno, 0);
sivachandraUnsubmitted
Not Done
ReplyInline Actions

Looks like these were added in https://reviews.llvm.org/D139576. If there are incorrect, they should be removed. If they are correct and the fix is expected to land separately, then please file a bug explaining the expected behavior in detail and add a TODO here which links to that bug.

sivachandra: Looks like these were added in https://reviews.llvm.org/D139576. If there are incorrect, they…
} }
TEST(LlvmLibcFILETest, FFlush) { TEST(LlvmLibcFILETest, FFlush) {
constexpr char FILENAME[] = "testdata/fflush.test"; constexpr char FILENAME[] = "testdata/fflush.test";
::FILE *file = __llvm_libc::fopen(FILENAME, "w+"); ::FILE *file = __llvm_libc::fopen(FILENAME, "w+");
ASSERT_FALSE(file == nullptr); ASSERT_FALSE(file == nullptr);
constexpr char CONTENT[] = "1234567890987654321"; constexpr char CONTENT[] = "1234567890987654321";
ASSERT_EQ(sizeof(CONTENT), ASSERT_EQ(sizeof(CONTENT),
▲ Show 20 Lines • Show All 53 Lines • Show Last 20 Lines

libc/test/src/stdio/ftell_test.cpp

Show First 20 Lines • Show All 49 Lines • ▼ Show 20 Lines void test_with_bufmode(int bufmode) {
char data[READ_SIZE]; char data[READ_SIZE];
// Reading a small amount will actually read out much more data and // Reading a small amount will actually read out much more data and
// buffer it. But, ftell should return the correct effective offset. // buffer it. But, ftell should return the correct effective offset.
ASSERT_EQ(READ_SIZE, __llvm_libc::fread(data, 1, READ_SIZE, file)); ASSERT_EQ(READ_SIZE, __llvm_libc::fread(data, 1, READ_SIZE, file));
ASSERT_EQ(size_t(__llvm_libc::ftell(file)), READ_SIZE); ASSERT_EQ(size_t(__llvm_libc::ftell(file)), READ_SIZE);
ASSERT_EQ(0, __llvm_libc::fclose(file)); ASSERT_EQ(0, __llvm_libc::fclose(file));
errno = 0; // errno = 0;
ASSERT_EQ(__llvm_libc::ftell(file), long(-1)); // ASSERT_EQ(__llvm_libc::ftell(file), long(-1));
ASSERT_NE(errno, 0); // ASSERT_NE(errno, 0);
} }
}; };
TEST_F(LlvmLibcFTellTest, TellWithFBF) { test_with_bufmode(_IOFBF); } TEST_F(LlvmLibcFTellTest, TellWithFBF) { test_with_bufmode(_IOFBF); }
TEST_F(LlvmLibcFTellTest, TellWithNBF) { test_with_bufmode(_IONBF); } TEST_F(LlvmLibcFTellTest, TellWithNBF) { test_with_bufmode(_IONBF); }
TEST_F(LlvmLibcFTellTest, TellWithLBF) { test_with_bufmode(_IOLBF); } TEST_F(LlvmLibcFTellTest, TellWithLBF) { test_with_bufmode(_IOLBF); }