[haiku-commits] r35743 - haiku/trunk/src/add-ons/kernel/file_systems/bfs

  • From: axeld@xxxxxxxxxxxxxxxx
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Wed, 3 Mar 2010 20:38:57 +0100 (CET)

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:
                {


Other related posts:

  • » [haiku-commits] r35743 - haiku/trunk/src/add-ons/kernel/file_systems/bfs - axeld