[haiku-development] Re: CID 702320 & 1397511 USE_AFTER_FREE patch

  • From: Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
  • To: haiku-development@xxxxxxxxxxxxx
  • Date: Thu, 29 Dec 2016 08:51:43 +0100

On Wed, Dec 28, 2016 at 03:52:09PM -0800, John Scipione wrote:

    Make sure to incriment the node's ref_count before calling
    vnode_path_to_vnode() which always decriments the nodes

Orthograph: increment/decrement, not incriment/decriment (several places
in comments too)

diff --git a/src/tools/fs_shell/vfs.cpp b/src/tools/fs_shell/vfs.cpp
index 5137ddd..cb7e654 100644
--- a/src/tools/fs_shell/vfs.cpp
+++ b/src/tools/fs_shell/vfs.cpp
@@ -1054,7 +1054,14 @@ entry_ref_to_vnode(fssh_mount_id mountID,
fssh_vnode_id directoryID, const char
  if (status < 0)
  return status;

- return vnode_path_to_vnode(directory, clonedName, false, 0, _vnode, NULL);
+ inc_vnode_ref_count(directory);
+ // incriment directory ref_count before calling vnode_path_to_vnode()
+ status = vnode_path_to_vnode(directory, clonedName, false, 0, _vnode, NULL);
+ // vnode_path_to_vnode() decriments directory ref_count
+ put_vnode(directory);
+ // decriment directory ref_count again, freeing it but not _vnode

This change looks useless, the vnode is freed immediately anyway.

- if (isDir)
+ if (isDir) {
+ inc_vnode_ref_count(dirNode);
  error = vnode_path_to_vnode(dirNode, leaf, false, 0, &dirNode, NULL);
- if (error != FSSH_B_OK) {
- TRACE(("vfs_normalize_path(): failed to get dir vnode for \".\" or
\"..\": %s\n",
- strerror(error)));
- return error;
+ // vnode_path_to_vnode() decriments dirNode ref_count

I wonder if it is possible to make vnode_path_to_vnode "balanced" (not
changing the ref_count inside it). I guess it was meant to be used as a
way to "convert directory to one of its leaves", which can be useful
when browsing the directory hierarchy, but it is now used in cases where
the directory is still used after calling it, so that would need to be
redesigned.

It would make the API behave more in the expected way, and would avoid
that the error happens again in the future.

  // get the directory path
  error = dir_vnode_to_path(dirNode, buffer, bufferSize);
- put_vnode(dirNode);
+ if (dirNode != NULL) {
+ // we don't need the vnode anymore
+ put_vnode(dirNode);
+ }
  if (error < FSSH_B_OK) {
  TRACE(("vfs_normalize_path(): failed to get dir path: %s\n",
strerror(error)));
  return error;
@@ -2749,8 +2762,10 @@ vfs_entry_ref_to_path(fssh_dev_t device,
fssh_ino_t inode, const char *leaf,

  // get the directory path
  status = dir_vnode_to_path(vnode, path, pathLength);
- put_vnode(vnode);
+ if (vnode != NULL) {
  // we don't need the vnode anymore
+ put_vnode(vnode);
+ }
  if (status < FSSH_B_OK)
  return status;

I would say vnode can't be null at these points. It isn't checked
directly, but there is a check for "status" before which make sure the
functions initializing it ran properly. If you want to check anyway, an
assert() would be a better choice here, I think.

-- 
Adrien.

Other related posts:

  1. Home
  2. haiku-development
  3. Archive
  4. 12-2016