* 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.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@35743 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp

@@ -1177,7 +1177,7 @@ BlockAllocator::_CheckGroup(int32 groupIndex) const
 
 
 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 @@ BlockAllocator::_IsValidCheckControl(check_control* control)
 
 
 status_t
-BlockAllocator::StartChecking(check_control* control)
+BlockAllocator::StartChecking(const check_control* control)
 {
 	if (!_IsValidCheckControl(control))
 		return B_BAD_VALUE;
@@ -1212,8 +1212,8 @@ BlockAllocator::StartChecking(check_control* control)
 		return B_NO_MEMORY;
 	}
 
-	check_cookie* cookie = new check_cookie();
+	fCheckCookie = new check_cookie();
-	if (cookie == NULL) {
+	if (fCheckCookie == NULL) {
 		free(fCheckBitmap);
 		fCheckBitmap = NULL;
 		mutex_unlock(&fLock);
@@ -1229,19 +1229,15 @@ BlockAllocator::StartChecking(check_control* control)
 		_SetCheckBitmapAt(block);
 	}
 
-	cookie->stack.Push(fVolume->Root());
+	fCheckCookie->stack.Push(fVolume->Root());
-	cookie->stack.Push(fVolume->Indices());
+	fCheckCookie->stack.Push(fVolume->Indices());
-	cookie->iterator = NULL;
+	fCheckCookie->iterator = NULL;
-	control->cookie = cookie;
-
-	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 @@ BlockAllocator::StartChecking(check_control* control)
 status_t
 BlockAllocator::StopChecking(check_control* control)
 {
-	check_cookie* cookie;
+	if (fCheckCookie == NULL)
-	if (control == NULL)
-		cookie = fCheckCookie;
-	else
-		cookie = (check_cookie*)control->cookie;
-
-	if (cookie == NULL)
 		return B_ERROR;
 
-	if (cookie->iterator != NULL) {
+	if (fCheckCookie->iterator != NULL) {
-		delete cookie->iterator;
+		delete fCheckCookie->iterator;
-		cookie->iterator = NULL;
+		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 @@ BlockAllocator::StopChecking(check_control* control)
 
 	free(fCheckBitmap);
 	fCheckBitmap = NULL;
+	delete fCheckCookie;
 	fCheckCookie = NULL;
-	delete cookie;
 	mutex_unlock(&fLock);
 	fVolume->GetJournal(0)->Unlock(NULL, true);
 
@@ -1366,22 +1356,21 @@ BlockAllocator::CheckNextNode(check_control* control)
 	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 (fCheckCookie->iterator == NULL) {
-			if (!cookie->stack.Pop(&cookie->current)) {
+			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 @@ BlockAllocator::CheckNextNode(check_control* control)
 			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;
+			fCheckCookie->parent = inode;
-			cookie->parent_mode = inode->Mode();
+			fCheckCookie->parent_mode = inode->Mode();
 
-			cookie->iterator = new TreeIterator(tree);
+			fCheckCookie->iterator = new TreeIterator(tree);
-			if (cookie->iterator == NULL)
+			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 @@ BlockAllocator::CheckNextNode(check_control* control)
 		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;
+			delete fCheckCookie->iterator;
-			cookie->iterator = NULL;
+			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 @@ BlockAllocator::CheckNextNode(check_control* control)
 				control->errors |= BFS_COULD_NOT_OPEN;
 
 				if ((control->flags & BFS_REMOVE_INVALID) != 0) {
-					status = _RemoveInvalidNode(cookie->parent,
+					status = _RemoveInvalidNode(fCheckCookie->parent,
-						cookie->iterator->Tree(), NULL, name);
+						fCheckCookie->iterator->Tree(), NULL, name);
 				} else
 					status = B_ERROR;
 
@@ -1501,22 +1491,22 @@ BlockAllocator::CheckNextNode(check_control* control)
 
 			// 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,
+					inode->Mode(), fCheckCookie->parent_mode,
-					cookie->parent->BlockNumber()));
+					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,
+					status = _RemoveInvalidNode(fCheckCookie->parent, NULL,
-						name);
+						inode, name);
 				} else
 					status = B_ERROR;
 
@@ -1527,7 +1517,7 @@ BlockAllocator::CheckNextNode(check_control* control)
 
 			// 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 @@ BlockAllocator::CheckInode(Inode* inode, check_control* control)
 		return status;
 
 	// If the inode has an attribute directory, push it on the stack
-	if (!inode->Attributes().IsZero()) {
+	if (!inode->Attributes().IsZero())
-		check_cookie* cookie = (check_cookie*)control->cookie;
+		fCheckCookie->stack.Push(inode->Attributes());
-		cookie->stack.Push(inode->Attributes());
-	}
 
 	if (inode->IsSymLink() && (inode->Flags() & INODE_LONG_SYMLINK) == 0) {
 		// symlinks may not have a valid data stream

src/add-ons/kernel/file_systems/bfs/BlockAllocator.h

@@ -46,7 +46,7 @@ public:
 								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 @@ private:
 #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);
 

src/add-ons/kernel/file_systems/bfs/bfs_control.h

@@ -51,7 +51,6 @@ struct check_control {
 		uint64	freed;
 	} stats;
 	status_t	status;
-	void*		cookie;
 };
 
 /* values for the flags field */

src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp

@@ -618,23 +618,21 @@ bfs_ioctl(fs_volume* _volume, fs_vnode* _node, void* _cookie, ulong cmd,
 
 	Volume* volume = (Volume*)_volume->private_volume;
 
-	// TODO: Access user buffers safely!
-
 	switch (cmd) {
 		case BFS_IOCTL_VERSION:
 		{
-			uint32 *version = (uint32*)buffer;
+			uint32 version = 0x10000;
-
+			return user_memcpy(buffer, &version, sizeof(uint32));
-			*version = 0x10000;
-			return B_OK;
 		}
 		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 @@ bfs_ioctl(fs_volume* _volume, fs_vnode* _node, void* _cookie, ulong cmd,
 		{
 			// 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 @@ bfs_ioctl(fs_volume* _volume, fs_vnode* _node, void* _cookie, ulong cmd,
 		{
 			// 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:
 		{