From 8f03af00f8119491a5d79d1a9859f2247a6d2bcf Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Thu, 18 Nov 2021 16:00:23 -0500 Subject: [PATCH] Storage: Rework LongDirEntry to be a union. Our dirent structure is "slim": it has a flexible-length array at the end which must be allocated to whatever size the consumer wants. However, we use [1] there and not [0] or [], which meant GCC thought it was not a flexible-length array, and so it optimized various string accesses that it assumed must be always false. Among these was BDirectory's check for "." and "..", and so that resulted in infinite loops. When changing our dirent structure to a proper FLA instead of [1], GCC then throws errors on LongDirEntry as it has data "after" the FLA; which is what we want, but there is no way to tell GCC that. So now we use a union instead, which is the proper way to statically allocate a FLA. This is part of #17389, but the real fix requires changing our dirent structure, which is coming in a separate commit. --- headers/private/storage/storage_support.h | 5 ++++- src/build/libbe/storage/Directory.cpp | 18 ++++++++------- src/build/libbe/storage/MergedDirectory.cpp | 7 +++--- src/build/libbe/storage/Node.cpp | 7 +++--- src/kits/storage/Directory.cpp | 22 ++++++++++--------- src/kits/storage/MergedDirectory.cpp | 11 +++++----- src/kits/storage/Node.cpp | 7 +++--- src/kits/storage/Query.cpp | 15 +++++++------ .../tracker/VirtualDirectoryEntryList.cpp | 11 +++++----- src/kits/tracker/VirtualDirectoryPoseView.cpp | 15 +++++++------ src/system/boot/loader/vfs.cpp | 6 +++-- 11 files changed, 70 insertions(+), 54 deletions(-) diff --git a/headers/private/storage/storage_support.h b/headers/private/storage/storage_support.h index 309aab6225..c3a7a4449b 100644 --- a/headers/private/storage/storage_support.h +++ b/headers/private/storage/storage_support.h @@ -25,7 +25,10 @@ namespace BPrivate { namespace Storage { // For convenience: -struct LongDirEntry : dirent { char _buffer[B_FILE_NAME_LENGTH]; }; +union LongDirEntry { + struct dirent dirent; + char _[sizeof(struct dirent) + B_PATH_NAME_LENGTH]; +}; //! Returns whether the supplied path is absolute. bool is_absolute_path(const char *path); diff --git a/src/build/libbe/storage/Directory.cpp b/src/build/libbe/storage/Directory.cpp index f8b1043fcb..37e0f625cc 100644 --- a/src/build/libbe/storage/Directory.cpp +++ b/src/build/libbe/storage/Directory.cpp @@ -332,19 +332,20 @@ BDirectory::GetNextRef(entry_ref* ref) if (InitCheck() != B_OK) return B_FILE_ERROR; - BPrivate::Storage::LongDirEntry entry; + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; bool next = true; while (next) { - if (GetNextDirents(&entry, sizeof(entry), 1) != 1) + if (GetNextDirents(entry, sizeof(longEntry), 1) != 1) return B_ENTRY_NOT_FOUND; - next = (!strcmp(entry.d_name, ".") - || !strcmp(entry.d_name, "..")); + next = (!strcmp(entry->d_name, ".") + || !strcmp(entry->d_name, "..")); } ref->device = fDirNodeRef.device; ref->directory = fDirNodeRef.node; - return ref->set_name(entry.d_name); + return ref->set_name(entry->d_name); } @@ -375,11 +376,12 @@ BDirectory::CountEntries() if (error != B_OK) return error; int32 count = 0; - BPrivate::Storage::LongDirEntry entry; + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; while (error == B_OK) { - if (GetNextDirents(&entry, sizeof(entry), 1) != 1) + if (GetNextDirents(entry, sizeof(longEntry), 1) != 1) break; - if (strcmp(entry.d_name, ".") != 0 && strcmp(entry.d_name, "..") != 0) + if (strcmp(entry->d_name, ".") != 0 && strcmp(entry->d_name, "..") != 0) count++; } Rewind(); diff --git a/src/build/libbe/storage/MergedDirectory.cpp b/src/build/libbe/storage/MergedDirectory.cpp index cbbbaad0dd..e8a566bbde 100644 --- a/src/build/libbe/storage/MergedDirectory.cpp +++ b/src/build/libbe/storage/MergedDirectory.cpp @@ -109,8 +109,9 @@ BMergedDirectory::GetNextEntry(BEntry* entry, bool traverse) status_t BMergedDirectory::GetNextRef(entry_ref* ref) { - BPrivate::Storage::LongDirEntry dirEntry; - int32 result = GetNextDirents(&dirEntry, sizeof(dirEntry), 1); + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* dirEntry = &longEntry.dirent; + int32 result = GetNextDirents(dirEntry, sizeof(longEntry), 1); if (result < 0) return result; if (result == 0) @@ -118,7 +119,7 @@ BMergedDirectory::GetNextRef(entry_ref* ref) BEntry entry; status_t error - = entry.SetTo(fDirectories.ItemAt(fDirectoryIndex), dirEntry.d_name); + = entry.SetTo(fDirectories.ItemAt(fDirectoryIndex), dirEntry->d_name); if (error != B_OK) return error; diff --git a/src/build/libbe/storage/Node.cpp b/src/build/libbe/storage/Node.cpp index 971c21f267..4c395c6b09 100644 --- a/src/build/libbe/storage/Node.cpp +++ b/src/build/libbe/storage/Node.cpp @@ -309,15 +309,16 @@ BNode::GetNextAttrName(char* buffer) if (InitAttrDir() != B_OK) return B_FILE_ERROR; - BPrivate::Storage::LongDirEntry entry; - ssize_t result = _kern_read_dir(fAttrFd, &entry, sizeof(entry), 1); + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; + ssize_t result = _kern_read_dir(fAttrFd, entry, sizeof(longEntry), 1); if (result < 0) return result; if (result == 0) return B_ENTRY_NOT_FOUND; - strlcpy(buffer, entry.d_name, B_ATTR_NAME_LENGTH); + strlcpy(buffer, entry->d_name, B_ATTR_NAME_LENGTH); return B_OK; } diff --git a/src/kits/storage/Directory.cpp b/src/kits/storage/Directory.cpp index 2b3d2051f1..048f1cbf7c 100644 --- a/src/kits/storage/Directory.cpp +++ b/src/kits/storage/Directory.cpp @@ -352,19 +352,20 @@ BDirectory::GetNextRef(entry_ref* ref) if (InitCheck() != B_OK) return B_FILE_ERROR; - BPrivate::Storage::LongDirEntry entry; + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; bool next = true; while (next) { - if (GetNextDirents(&entry, sizeof(entry), 1) != 1) + if (GetNextDirents(entry, sizeof(longEntry), 1) != 1) return B_ENTRY_NOT_FOUND; - next = (!strcmp(entry.d_name, ".") - || !strcmp(entry.d_name, "..")); + next = (!strcmp(entry->d_name, ".") + || !strcmp(entry->d_name, "..")); } - ref->device = entry.d_pdev; - ref->directory = entry.d_pino; - return ref->set_name(entry.d_name); + ref->device = entry->d_pdev; + ref->directory = entry->d_pino; + return ref->set_name(entry->d_name); } @@ -395,11 +396,12 @@ BDirectory::CountEntries() if (error != B_OK) return error; int32 count = 0; - BPrivate::Storage::LongDirEntry entry; + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; while (error == B_OK) { - if (GetNextDirents(&entry, sizeof(entry), 1) != 1) + if (GetNextDirents(entry, sizeof(longEntry), 1) != 1) break; - if (strcmp(entry.d_name, ".") != 0 && strcmp(entry.d_name, "..") != 0) + if (strcmp(entry->d_name, ".") != 0 && strcmp(entry->d_name, "..") != 0) count++; } Rewind(); diff --git a/src/kits/storage/MergedDirectory.cpp b/src/kits/storage/MergedDirectory.cpp index 90519ed3e8..5eb4f25d38 100644 --- a/src/kits/storage/MergedDirectory.cpp +++ b/src/kits/storage/MergedDirectory.cpp @@ -111,16 +111,17 @@ BMergedDirectory::GetNextEntry(BEntry* entry, bool traverse) status_t BMergedDirectory::GetNextRef(entry_ref* ref) { - BPrivate::Storage::LongDirEntry entry; - int32 result = GetNextDirents(&entry, sizeof(entry), 1); + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; + int32 result = GetNextDirents(entry, sizeof(longEntry), 1); if (result < 0) return result; if (result == 0) return B_ENTRY_NOT_FOUND; - ref->device = entry.d_pdev; - ref->directory = entry.d_pino; - return ref->set_name(entry.d_name); + ref->device = entry->d_pdev; + ref->directory = entry->d_pino; + return ref->set_name(entry->d_name); } diff --git a/src/kits/storage/Node.cpp b/src/kits/storage/Node.cpp index c04de67591..08bd6d3786 100644 --- a/src/kits/storage/Node.cpp +++ b/src/kits/storage/Node.cpp @@ -319,15 +319,16 @@ BNode::GetNextAttrName(char* buffer) if (InitAttrDir() != B_OK) return B_FILE_ERROR; - BPrivate::Storage::LongDirEntry entry; - ssize_t result = _kern_read_dir(fAttrFd, &entry, sizeof(entry), 1); + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; + ssize_t result = _kern_read_dir(fAttrFd, entry, sizeof(longEntry), 1); if (result < 0) return result; if (result == 0) return B_ENTRY_NOT_FOUND; - strlcpy(buffer, entry.d_name, B_ATTR_NAME_LENGTH); + strlcpy(buffer, entry->d_name, B_ATTR_NAME_LENGTH); return B_OK; } diff --git a/src/kits/storage/Query.cpp b/src/kits/storage/Query.cpp index 20244f61f1..4719d1cf4b 100644 --- a/src/kits/storage/Query.cpp +++ b/src/kits/storage/Query.cpp @@ -348,20 +348,21 @@ BQuery::GetNextRef(entry_ref* ref) if (error == B_OK && !_HasFetched()) error = B_FILE_ERROR; if (error == B_OK) { - BPrivate::Storage::LongDirEntry entry; + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; bool next = true; while (error == B_OK && next) { - if (GetNextDirents(&entry, sizeof(entry), 1) != 1) { + if (GetNextDirents(entry, sizeof(longEntry), 1) != 1) { error = B_ENTRY_NOT_FOUND; } else { - next = (!strcmp(entry.d_name, ".") - || !strcmp(entry.d_name, "..")); + next = (!strcmp(entry->d_name, ".") + || !strcmp(entry->d_name, "..")); } } if (error == B_OK) { - ref->device = entry.d_pdev; - ref->directory = entry.d_pino; - error = ref->set_name(entry.d_name); + ref->device = entry->d_pdev; + ref->directory = entry->d_pino; + error = ref->set_name(entry->d_name); } } return error; diff --git a/src/kits/tracker/VirtualDirectoryEntryList.cpp b/src/kits/tracker/VirtualDirectoryEntryList.cpp index 067e967e89..1b9c8bdcd9 100644 --- a/src/kits/tracker/VirtualDirectoryEntryList.cpp +++ b/src/kits/tracker/VirtualDirectoryEntryList.cpp @@ -82,16 +82,17 @@ VirtualDirectoryEntryList::GetNextEntry(BEntry* entry, bool traverse) status_t VirtualDirectoryEntryList::GetNextRef(entry_ref* ref) { - BPrivate::Storage::LongDirEntry entry; - int32 result = GetNextDirents(&entry, sizeof(entry), 1); + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; + int32 result = GetNextDirents(entry, sizeof(longEntry), 1); if (result < 0) return result; if (result == 0) return B_ENTRY_NOT_FOUND; - ref->device = entry.d_pdev; - ref->directory = entry.d_pino; - return ref->set_name(entry.d_name); + ref->device = entry->d_pdev; + ref->directory = entry->d_pino; + return ref->set_name(entry->d_name); } diff --git a/src/kits/tracker/VirtualDirectoryPoseView.cpp b/src/kits/tracker/VirtualDirectoryPoseView.cpp index a666626ef1..0ffde695d9 100644 --- a/src/kits/tracker/VirtualDirectoryPoseView.cpp +++ b/src/kits/tracker/VirtualDirectoryPoseView.cpp @@ -208,14 +208,15 @@ VirtualDirectoryPoseView::_EntryCreated(const BMessage* message) if (directory.SetTo(&nodeRef) != B_OK) return true; - BPrivate::Storage::LongDirEntry entry; - while (directory.GetNextDirents(&entry, sizeof(entry), 1) == 1) { - if (strcmp(entry.d_name, ".") != 0 - && strcmp(entry.d_name, "..") != 0) { + BPrivate::Storage::LongDirEntry longEntry; + struct dirent* entry = &longEntry.dirent; + while (directory.GetNextDirents(entry, sizeof(longEntry), 1) == 1) { + if (strcmp(entry->d_name, ".") != 0 + && strcmp(entry->d_name, "..") != 0) { _DispatchEntryCreatedOrRemovedMessage(B_ENTRY_CREATED, - node_ref(entry.d_dev, entry.d_ino), - NotOwningEntryRef(entry.d_pdev, entry.d_pino, - entry.d_name), + node_ref(entry->d_dev, entry->d_ino), + NotOwningEntryRef(entry->d_pdev, entry->d_pino, + entry->d_name), NULL, false); } } diff --git a/src/system/boot/loader/vfs.cpp b/src/system/boot/loader/vfs.cpp index 150758ec54..d0fc94b2fb 100644 --- a/src/system/boot/loader/vfs.cpp +++ b/src/system/boot/loader/vfs.cpp @@ -42,8 +42,10 @@ using namespace boot; struct __DIR { Directory* directory; void* cookie; - dirent entry; - char nameBuffer[B_FILE_NAME_LENGTH - 1]; + union { + dirent entry; + char nameBuffer[sizeof(dirent) + B_FILE_NAME_LENGTH - 1]; + }; };