Author: axeld Date: 2010-03-03 20:38:56 +0100 (Wed, 03 Mar 2010) New Revision: 35743 Changeset: http://dev.haiku-os.org/changeset/35743/haiku Ticket: http://dev.haiku-os.org/ticket/3264 Modified: haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h haiku/trunk/src/add-ons/kernel/file_systems/bfs/bfs_control.h haiku/trunk/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp Log: * Removed the cookie field of the check_control structure; that really was a stupid idea. Instead, the already existing fCheckCookie member is used. * bfs_ioctl() now accesses all userland buffers safely, this should help with #3264, and move the crash where it belongs. * Changes not yet tested; they only affect checkfs. Modified: haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp =================================================================== --- haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp 2010-03-03 19:20:51 UTC (rev 35742) +++ haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp 2010-03-03 19:38:56 UTC (rev 35743) @@ -1177,7 +1177,7 @@ bool -BlockAllocator::_IsValidCheckControl(check_control* control) +BlockAllocator::_IsValidCheckControl(const check_control* control) { if (control == NULL || control->magic != BFS_IOCTL_CHECK_MAGIC) { @@ -1190,7 +1190,7 @@ status_t -BlockAllocator::StartChecking(check_control* control) +BlockAllocator::StartChecking(const check_control* control) { if (!_IsValidCheckControl(control)) return B_BAD_VALUE; @@ -1212,8 +1212,8 @@ return B_NO_MEMORY; } - check_cookie* cookie = new check_cookie(); - if (cookie == NULL) { + fCheckCookie = new check_cookie(); + if (fCheckCookie == NULL) { free(fCheckBitmap); fCheckBitmap = NULL; mutex_unlock(&fLock); @@ -1229,19 +1229,15 @@ _SetCheckBitmapAt(block); } - cookie->stack.Push(fVolume->Root()); - cookie->stack.Push(fVolume->Indices()); - cookie->iterator = NULL; - control->cookie = cookie; + fCheckCookie->stack.Push(fVolume->Root()); + fCheckCookie->stack.Push(fVolume->Indices()); + fCheckCookie->iterator = NULL; - fCheckCookie = cookie; - // to be able to restore nicely if "chkbfs" exited abnormally - // Put removed vnodes to the stack -- they are not reachable by traversing // the file system anymore. InodeList::Iterator iterator = fVolume->RemovedInodes().GetIterator(); while (Inode* inode = iterator.Next()) { - cookie->stack.Push(inode->BlockRun()); + fCheckCookie->stack.Push(inode->BlockRun()); } // TODO: check reserved area in bitmap! @@ -1253,21 +1249,15 @@ status_t BlockAllocator::StopChecking(check_control* control) { - check_cookie* cookie; - if (control == NULL) - cookie = fCheckCookie; - else - cookie = (check_cookie*)control->cookie; - - if (cookie == NULL) + if (fCheckCookie == NULL) return B_ERROR; - if (cookie->iterator != NULL) { - delete cookie->iterator; - cookie->iterator = NULL; + if (fCheckCookie->iterator != NULL) { + delete fCheckCookie->iterator; + fCheckCookie->iterator = NULL; // the current directory inode is still locked in memory - put_vnode(fVolume->FSVolume(), fVolume->ToVnode(cookie->current)); + put_vnode(fVolume->FSVolume(), fVolume->ToVnode(fCheckCookie->current)); } if (fVolume->IsReadOnly()) { @@ -1351,8 +1341,8 @@ free(fCheckBitmap); fCheckBitmap = NULL; + delete fCheckCookie; fCheckCookie = NULL; - delete cookie; mutex_unlock(&fLock); fVolume->GetJournal(0)->Unlock(NULL, true); @@ -1366,22 +1356,21 @@ if (!_IsValidCheckControl(control)) return B_BAD_VALUE; - check_cookie* cookie = (check_cookie*)control->cookie; fVolume->SetCheckingThread(find_thread(NULL)); while (true) { - if (cookie->iterator == NULL) { - if (!cookie->stack.Pop(&cookie->current)) { + if (fCheckCookie->iterator == NULL) { + if (!fCheckCookie->stack.Pop(&fCheckCookie->current)) { // no more runs on the stack, we are obviously finished! control->status = B_ENTRY_NOT_FOUND; return B_ENTRY_NOT_FOUND; } - Vnode vnode(fVolume, cookie->current); + Vnode vnode(fVolume, fCheckCookie->current); Inode* inode; if (vnode.Get(&inode) != B_OK) { FATAL(("check: Could not open inode at %" B_PRIdOFF "\n", - fVolume->ToBlock(cookie->current))); + fVolume->ToBlock(fCheckCookie->current))); continue; } @@ -1404,15 +1393,15 @@ BPlusTree* tree = inode->Tree(); if (tree == NULL) { FATAL(("check: could not open b+tree from inode at %" B_PRIdOFF - "\n", fVolume->ToBlock(cookie->current))); + "\n", fVolume->ToBlock(fCheckCookie->current))); continue; } - cookie->parent = inode; - cookie->parent_mode = inode->Mode(); + fCheckCookie->parent = inode; + fCheckCookie->parent_mode = inode->Mode(); - cookie->iterator = new TreeIterator(tree); - if (cookie->iterator == NULL) + fCheckCookie->iterator = new TreeIterator(tree); + if (fCheckCookie->iterator == NULL) RETURN_ERROR(B_NO_MEMORY); // the inode must stay locked in memory until the iterator is freed @@ -1432,15 +1421,16 @@ uint16 length; ino_t id; - status_t status = cookie->iterator->GetNextEntry(name, &length, + status_t status = fCheckCookie->iterator->GetNextEntry(name, &length, B_FILE_NAME_LENGTH, &id); if (status == B_ENTRY_NOT_FOUND) { // there are no more entries in this iterator, free it and go on - delete cookie->iterator; - cookie->iterator = NULL; + delete fCheckCookie->iterator; + fCheckCookie->iterator = NULL; // unlock the directory's inode from memory - put_vnode(fVolume->FSVolume(), fVolume->ToVnode(cookie->current)); + put_vnode(fVolume->FSVolume(), + fVolume->ToVnode(fCheckCookie->current)); continue; } else if (status == B_OK) { @@ -1460,8 +1450,8 @@ control->errors |= BFS_COULD_NOT_OPEN; if ((control->flags & BFS_REMOVE_INVALID) != 0) { - status = _RemoveInvalidNode(cookie->parent, - cookie->iterator->Tree(), NULL, name); + status = _RemoveInvalidNode(fCheckCookie->parent, + fCheckCookie->iterator->Tree(), NULL, name); } else status = B_ERROR; @@ -1501,22 +1491,22 @@ // Check for the correct mode of the node (if the mode of the // file don't fit to its parent, there is a serious problem) - if (((cookie->parent_mode & S_ATTR_DIR) != 0 + if (((fCheckCookie->parent_mode & S_ATTR_DIR) != 0 && !inode->IsAttribute()) - || ((cookie->parent_mode & S_INDEX_DIR) != 0 + || ((fCheckCookie->parent_mode & S_INDEX_DIR) != 0 && !inode->IsIndex()) - || (is_directory(cookie->parent_mode) + || (is_directory(fCheckCookie->parent_mode) && !inode->IsRegularNode())) { FATAL(("inode at %" B_PRIdOFF " is of wrong type: %o (parent " "%o at %" B_PRIdOFF ")!\n", inode->BlockNumber(), - inode->Mode(), cookie->parent_mode, - cookie->parent->BlockNumber())); + inode->Mode(), fCheckCookie->parent_mode, + fCheckCookie->parent->BlockNumber())); // if we are allowed to fix errors, we should remove the file if ((control->flags & BFS_REMOVE_WRONG_TYPES) != 0 && (control->flags & BFS_FIX_BITMAP_ERRORS) != 0) { - status = _RemoveInvalidNode(cookie->parent, NULL, inode, - name); + status = _RemoveInvalidNode(fCheckCookie->parent, NULL, + inode, name); } else status = B_ERROR; @@ -1527,7 +1517,7 @@ // push the directory on the stack so that it will be scanned later if (inode->IsContainer() && !inode->IsIndex()) - cookie->stack.Push(inode->BlockRun()); + fCheckCookie->stack.Push(inode->BlockRun()); else { // check it now control->status = CheckInode(inode, control); @@ -1757,10 +1747,8 @@ return status; // If the inode has an attribute directory, push it on the stack - if (!inode->Attributes().IsZero()) { - check_cookie* cookie = (check_cookie*)control->cookie; - cookie->stack.Push(inode->Attributes()); - } + if (!inode->Attributes().IsZero()) + fCheckCookie->stack.Push(inode->Attributes()); if (inode->IsSymLink() && (inode->Flags() & INODE_LONG_SYMLINK) == 0) { // symlinks may not have a valid data stream Modified: haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h =================================================================== --- haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h 2010-03-03 19:20:51 UTC (rev 35742) +++ haiku/trunk/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h 2010-03-03 19:38:56 UTC (rev 35743) @@ -46,7 +46,7 @@ int32 group, uint16 start, uint16 numBlocks, uint16 minimum, block_run& run); - status_t StartChecking(check_control* control); + status_t StartChecking(const check_control* control); status_t StopChecking(check_control* control); status_t CheckNextNode(check_control* control); @@ -74,7 +74,7 @@ #ifdef DEBUG_ALLOCATION_GROUPS void _CheckGroup(int32 group) const; #endif - bool _IsValidCheckControl(check_control* control); + bool _IsValidCheckControl(const check_control* control); bool _CheckBitmapIsUsedAt(off_t block) const; void _SetCheckBitmapAt(off_t block); Modified: haiku/trunk/src/add-ons/kernel/file_systems/bfs/bfs_control.h =================================================================== --- haiku/trunk/src/add-ons/kernel/file_systems/bfs/bfs_control.h 2010-03-03 19:20:51 UTC (rev 35742) +++ haiku/trunk/src/add-ons/kernel/file_systems/bfs/bfs_control.h 2010-03-03 19:38:56 UTC (rev 35743) @@ -51,7 +51,6 @@ uint64 freed; } stats; status_t status; - void* cookie; }; /* values for the flags field */ Modified: haiku/trunk/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp =================================================================== --- haiku/trunk/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp 2010-03-03 19:20:51 UTC (rev 35742) +++ haiku/trunk/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp 2010-03-03 19:38:56 UTC (rev 35743) @@ -618,23 +618,21 @@ Volume* volume = (Volume*)_volume->private_volume; - // TODO: Access user buffers safely! - switch (cmd) { case BFS_IOCTL_VERSION: { - uint32 *version = (uint32*)buffer; - - *version = 0x10000; - return B_OK; + uint32 version = 0x10000; + return user_memcpy(buffer, &version, sizeof(uint32)); } case BFS_IOCTL_START_CHECKING: { // start checking BlockAllocator& allocator = volume->Allocator(); - check_control* control = (check_control*)buffer; + check_control control; + if (user_memcpy(&control, buffer, sizeof(check_control)) != B_OK) + return B_BAD_ADDRESS; - status_t status = allocator.StartChecking(control); + status_t status = allocator.StartChecking(&control); if (status == B_OK) { file_cookie* cookie = (file_cookie*)_cookie; cookie->open_mode |= BFS_OPEN_MODE_CHECKING; @@ -646,13 +644,15 @@ { // stop checking BlockAllocator& allocator = volume->Allocator(); - check_control* control = (check_control*)buffer; + check_control control; - status_t status = allocator.StopChecking(control); + status_t status = allocator.StopChecking(&control); if (status == B_OK) { file_cookie* cookie = (file_cookie*)_cookie; cookie->open_mode &= ~BFS_OPEN_MODE_CHECKING; } + if (status == B_OK) + status = user_memcpy(buffer, &control, sizeof(check_control)); return status; } @@ -660,9 +660,13 @@ { // check next BlockAllocator& allocator = volume->Allocator(); - check_control* control = (check_control*)buffer; + check_control control; - return allocator.CheckNextNode(control); + status_t status = allocator.CheckNextNode(&control); + if (status == B_OK) + status = user_memcpy(buffer, &control, sizeof(check_control)); + + return status; } case BFS_IOCTL_UPDATE_BOOT_BLOCK: {