[PATCH 0/5] btrfs-progs: btrfs path leak fix, part 1

[PATCH 0/5] btrfs-progs: btrfs path leak fix, part 1

[Qu Wenruo]

To avoid future LLM script kiddos, we need to fix path leaks.

This is part 1, focusing on leaks on btrfs_alloc_path().

The most important part is the second patch, which introduces
BTRFS_PATH_AUTO_(FREE|RELEASE), so that we can finally get the same
capability.

Unfortunately this will require the compiler support for the __cleanup__
attribute. If not supported, configure will reject the build.

Then several fixes using BTRFS_PATH_AUTO_FREE().

The last one has several other small fixes batched together, as they are
all inside the same btrfs-corrupt-block.

Part 2 will focus on the on-stack paths, which can still leak the ebs,
and are the most common sites in btrfs-progs.

Qu Wenruo (5):
  btrfs-progs: introduce compiler_attributes.h
  btrfs-progs: utilize __cleanup__ attribute for btrfs_path
  btrfs-progs: sync btrfs_translate_remap() to fix a path leak
  btrfs-progs: fix a path leak in alloc_reserved_tree_block()
  btrfs-progs: fix several memory bugs in btrfs-corrupt-block

 btrfs-corrupt-block.c         | 34 ++++++++-----
 configure.ac                  | 15 ++++++
 include/compiler_attributes.h | 39 +++++++++++++++
 include/kerncompat.h          |  7 +--
 kernel-lib/overflow.h         |  8 +---
 kernel-shared/cleanup.h       | 89 +++++++++++++++++++++++++++++++++++
 kernel-shared/ctree.h         | 10 ++++
 kernel-shared/extent-tree.c   |  4 +-
 kernel-shared/volumes.c       | 55 +++++-----------------
 9 files changed, 193 insertions(+), 68 deletions(-)
 create mode 100644 include/compiler_attributes.h
 create mode 100644 kernel-shared/cleanup.h

--
2.56.0

[PATCH 1/5] btrfs-progs: introduce compiler_attributes.h

[Qu Wenruo]

Cross-port a minimal include/compiler_attributes.h from the kernel's
include/linux/compiler_attributes.h, and move the existing attribute
definitions there:

- __must_check
  Previously defined in kernel-lib/overflow.h, which is not a proper
  place for a generic attribute. Anyone wanting __must_check would have
  to include overflow.h just for it.

- __printf
- __maybe_unused
  Previously defined in include/kerncompat.h.

The new header has no other includes so it can be pulled in by any
header that only needs a compiler attribute. kerncompat.h includes it,
so existing users are not affected.

Assisted-By: LLM (Split the compiler_attributes.h from the next commit)
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 include/compiler_attributes.h | 33 +++++++++++++++++++++++++++++++++
 include/kerncompat.h          |  7 +------
 kernel-lib/overflow.h         |  8 +-------
 3 files changed, 35 insertions(+), 13 deletions(-)
 create mode 100644 include/compiler_attributes.h

diff --git a/include/compiler_attributes.h b/include/compiler_attributes.h
new file mode 100644
index 000000000000..e9ac1ac64cd8
--- /dev/null
+++ b/include/compiler_attributes.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _COMPILER_ATTRIBUTES_H
+#define _COMPILER_ATTRIBUTES_H
+
+/*
+ * Copied from linux.git/include/linux/compiler_attributes.h, only contains the
+ * attributes btrfs-progs needs.
+ *
+ * This header must stay free of any other includes, so it can be pulled in by
+ * any header which only needs a compiler attribute.
+ */
+
+/*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-format-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#format
+ */
+#define __printf(a, b)			__attribute__((__format__(printf, a, b)))
+
+/*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-warn_005funused_005fresult-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
+ */
+#define __must_check			__attribute__((__warn_unused_result__))
+
+/*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-unused-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#maybe-unused-unused
+ */
+#ifndef __maybe_unused
+#define __maybe_unused			__attribute__((__unused__))
+#endif
+
+#endif
diff --git a/include/kerncompat.h b/include/kerncompat.h
index 0239b0ac807a..b8e9a6c9c04b 100644
--- a/include/kerncompat.h
+++ b/include/kerncompat.h
@@ -38,6 +38,7 @@
 #include <stdbool.h>
 #include <stdarg.h>
 #include <features.h>
+#include "compiler_attributes.h"
 
 #ifndef static_assert
 #define static_assert(expr, str) _Static_assert(expr, str)
@@ -58,10 +59,6 @@
 
 #endif
 
-#ifndef __maybe_unused
-#define __maybe_unused __attribute__((__unused__))
-#endif
-
 #ifndef BTRFS_DISABLE_BACKTRACE
 #include <execinfo.h>
 #endif
@@ -776,8 +773,6 @@ struct lock_class_key {
 #define __user
 #define __pure
 
-#define __printf(a, b)                  __attribute__((__format__(printf, a, b)))
-
 static inline bool sb_rdonly(struct super_block *sb)
 {
 	return false;
diff --git a/kernel-lib/overflow.h b/kernel-lib/overflow.h
index b2a376ee13c4..d942b11924cc 100644
--- a/kernel-lib/overflow.h
+++ b/kernel-lib/overflow.h
@@ -5,6 +5,7 @@
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
+#include "compiler_attributes.h"
 
 /*
  * We need to compute the minimum and maximum values representable in a given
@@ -42,13 +43,6 @@
 #define is_non_negative(a) ((a) > 0 || (a) == 0)
 #define is_negative(a) (!(is_non_negative(a)))
 
-/* Copied from linux.git/include/compiler_attributes.h */
-/*
- *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-warn_005funused_005fresult-function-attribute
- * clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
- */
-#define __must_check                    __attribute__((__warn_unused_result__))
-
 /*
  * Allows for effectively applying __must_check to a macro so we can have
  * both the type-agnostic benefits of the macros while also being able to
-- 
2.56.0

[PATCH 2/5] btrfs-progs: utilize __cleanup__ attribute for btrfs_path

[Qu Wenruo]

The new header at kernel-shared/cleanup.h is cross-ported from the
kernel's linux/cleanup.h.

Since we rely on the newer __cleanup__ attribute, also update the
configure tool to detect the compiler support.
If no support, error out.

As we will rely on the __cleanup__ attribute to do auto cleanup. Ignoring
such attribute will easily screw up future resource management.

With the new __free() macros, define BTRFS_PATH_AUTO_FREE() and
BTRFS_PATH_AUTO_RELEASE(), so we can sync the code between progs and
kernel.

Assisted-by: LLM (updating configure.ac to error out if no such support)
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 configure.ac                  | 15 ++++++
 include/compiler_attributes.h |  6 +++
 kernel-shared/cleanup.h       | 89 +++++++++++++++++++++++++++++++++++
 kernel-shared/ctree.h         | 10 ++++
 4 files changed, 120 insertions(+)
 create mode 100644 kernel-shared/cleanup.h

diff --git a/configure.ac b/configure.ac
index b12ca46814bf..07cd92df5947 100644
--- a/configure.ac
+++ b/configure.ac
@@ -67,6 +67,21 @@ AS_IF([test "x$target_cpu" = 'xx86_64'], [
 AX_CHECK_COMPILE_FLAG([-std=gnu11], [],
 	[AC_MSG_ERROR([std=gnu11 compiler required])])
 
+dnl The __cleanup__ variable attribute is required by kernel-shared/cleanup.h
+AC_MSG_CHECKING([whether the compiler supports __attribute__((__cleanup__))])
+save_ac_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
+static void free_int(int *p) { *p = 0; }
+]], [[
+	int x __attribute__((__cleanup__(free_int))) = 1;
+	(void)x;
+]])],
+	[AC_MSG_RESULT([yes])],
+	[AC_MSG_RESULT([no])
+	 AC_MSG_ERROR([compiler with __attribute__((__cleanup__)) support required])])
+ac_c_werror_flag=$save_ac_c_werror_flag
+
 AC_SYS_LARGEFILE
 
 AC_PROG_INSTALL
diff --git a/include/compiler_attributes.h b/include/compiler_attributes.h
index e9ac1ac64cd8..41edbab9c55a 100644
--- a/include/compiler_attributes.h
+++ b/include/compiler_attributes.h
@@ -10,6 +10,12 @@
  * any header which only needs a compiler attribute.
  */
 
+/*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#cleanup
+ */
+#define __cleanup(func)			__attribute__((__cleanup__(func)))
+
 /*
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-format-function-attribute
  * clang: https://clang.llvm.org/docs/AttributeReference.html#format
diff --git a/kernel-shared/cleanup.h b/kernel-shared/cleanup.h
new file mode 100644
index 000000000000..c8f12e2e343a
--- /dev/null
+++ b/kernel-shared/cleanup.h
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Cross-ported from kernel's include/linux/cleanup.h, along with needed
+ * compiler attributes from include/linux/compiler_attributes.h
+ */
+#ifndef _LINUX_CLEANUP_H
+#define _LINUX_CLEANUP_H
+
+#include "kerncompat.h"
+#include "compiler_attributes.h"
+
+/*
+ * DEFINE_FREE(name, type, free):
+ *	simple helper macro that defines the required wrapper for a __free()
+ *	based cleanup function. @free is an expression using '_T' to access the
+ *	variable. @free should typically include a NULL test before calling a
+ *	function, see the example below.
+ *
+ * __free(name):
+ *	variable attribute to add a scoped based cleanup to the variable.
+ *
+ * no_free_ptr(var):
+ *	like a non-atomic xchg(var, NULL), such that the cleanup function will
+ *	be inhibited -- provided it sanely deals with a NULL value.
+ *
+ *	NOTE: this has __must_check semantics so that it is harder to accidentally
+ *	leak the resource.
+ *
+ * return_ptr(p):
+ *	returns p while inhibiting the __free().
+ *
+ * Ex.
+ *
+ * DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
+ *
+ * void *alloc_obj(...)
+ * {
+ *	struct obj *p __free(kfree) = kmalloc(...);
+ *	if (!p)
+ *		return NULL;
+ *
+ *	if (!init_obj(p))
+ *		return NULL;
+ *
+ *	return_ptr(p);
+ * }
+ *
+ * NOTE: the DEFINE_FREE()'s @free expression includes a NULL test even though
+ * kfree() is fine to be called with a NULL value. This is on purpose. This way
+ * the compiler sees the end of our alloc_obj() function as:
+ *
+ *	tmp = p;
+ *	p = NULL;
+ *	if (p)
+ *		kfree(p);
+ *	return tmp;
+ *
+ * And through the magic of value-propagation and dead-code-elimination, it
+ * eliminates the actual cleanup call and compiles into:
+ *
+ *	return p;
+ *
+ * Without the NULL test it turns into a mess and the compiler can't help us.
+ */
+
+#define DEFINE_FREE(_name, _type, _free) \
+	static __always_inline void __free_##_name(void *p) { _type _T = *(_type *)p; _free; }
+
+#define __free(_name)	__cleanup(__free_##_name)
+
+#define __get_and_null(p, nullvalue)	    \
+	({                                  \
+		auto __ptr = &(p);	    \
+		auto __val = *__ptr;	    \
+		*__ptr = nullvalue;         \
+		__val;                      \
+	})
+
+static __always_inline __must_check
+const volatile void * __must_check_fn(const volatile void *val)
+{ return val; }
+
+#define no_free_ptr(p) \
+	((typeof(p)) __must_check_fn((__force const volatile void *)__get_and_null(p, NULL)))
+
+#define return_ptr(p)	return no_free_ptr(p)
+
+#endif
diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h
index 93fd7f18049e..3a5f456df315 100644
--- a/kernel-shared/ctree.h
+++ b/kernel-shared/ctree.h
@@ -30,6 +30,7 @@
 #include "kernel-shared/accessors.h"
 #include "kernel-shared/extent-io-tree.h"
 #include "kernel-shared/locking.h"
+#include "kernel-shared/cleanup.h"
 #include "crypto/crc32c.h"
 #include "common/extent-cache.h"
 
@@ -1024,6 +1025,15 @@ void btrfs_release_path(struct btrfs_path *p);
 void add_root_to_dirty_list(struct btrfs_root *root);
 struct btrfs_path *btrfs_alloc_path(void);
 void btrfs_free_path(struct btrfs_path *p);
+DEFINE_FREE(btrfs_free_path, struct btrfs_path *, btrfs_free_path(_T))
+DEFINE_FREE(btrfs_release_path, struct btrfs_path, btrfs_release_path(&_T))
+
+#define BTRFS_PATH_AUTO_FREE(path_name)					\
+	struct btrfs_path *path_name __free(btrfs_free_path) = NULL
+
+#define BTRFS_PATH_AUTO_RELEASE(path_name)				\
+	struct btrfs_path path_name __free(btrfs_release_path) = { 0 }
+
 int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		   struct btrfs_path *path, int slot, int nr);
 
-- 
2.56.0

[PATCH 3/5] btrfs-progs: sync btrfs_translate_remap() to fix a path leak

[Qu Wenruo]

In the current implementation, if btrfs_next_leaf() returned 1, the
function returned without freeing the path.

Fix it by syncing the code from kernel, which do a key search using
(*logical, -1, -1), then moving to the previous slot, which should be
safer than the current one.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/volumes.c | 55 ++++++++++-------------------------------
 1 file changed, 13 insertions(+), 42 deletions(-)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index e2b6708ad872..fedcaa5f9216 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -2098,75 +2098,46 @@ static int btrfs_translate_remap(struct btrfs_fs_info *fs_info, u64 *logical, u6
 	struct btrfs_key key, found_key;
 	struct extent_buffer *leaf;
 	struct btrfs_remap_item *remap;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 
 	path = btrfs_alloc_path();
 	if (!path)
 		return -ENOMEM;
 
 	key.objectid = *logical;
-	key.type = BTRFS_IDENTITY_REMAP_KEY;
-	key.offset = 0;
+	key.type = (u8)-1;
+	key.offset = (u64)-1;
 
 	ret = btrfs_search_slot(NULL, fs_info->remap_root, &key, path, 0, 0);
-	if (ret < 0) {
-		btrfs_free_path(path);
+	if (ret < 0)
 		return ret;
-	}
 
 	leaf = path->nodes[0];
+	if (path->slots[0] == 0)
+		return -ENOENT;
 
-	if (path->slots[0] >= btrfs_header_nritems(leaf)) {
-		ret = btrfs_next_leaf(fs_info->remap_root, path);
-		if (ret < 0) {
-			btrfs_free_path(path);
-			return ret;
-		}
-
-		leaf = path->nodes[0];
-	}
+	path->slots[0]--;
 
 	btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
 
-	if (found_key.objectid > *logical) {
-		if (path->slots[0] == 0) {
-			ret = btrfs_prev_leaf(fs_info->remap_root, path);
-			if (ret) {
-				if (ret == 1)
-					ret = -ENOENT;
-				return ret;
-			}
-
-			leaf = path->nodes[0];
-		} else {
-			path->slots[0]--;
-		}
-
-		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
-	}
-
-	if (found_key.type != BTRFS_REMAP_KEY && found_key.type != BTRFS_IDENTITY_REMAP_KEY) {
-		btrfs_free_path(path);
+	if (found_key.type != BTRFS_REMAP_KEY &&
+	    found_key.type != BTRFS_IDENTITY_REMAP_KEY) {
 		return -ENOENT;
 	}
 
-	if (found_key.objectid > *logical || found_key.objectid + found_key.offset <= *logical) {
-		btrfs_free_path(path);
+	if (found_key.objectid > *logical ||
+	    found_key.objectid + found_key.offset <= *logical) {
 		return -ENOENT;
 	}
 
 	if (*logical + *length > found_key.objectid + found_key.offset)
 		*length = found_key.objectid + found_key.offset - *logical;
 
-	if (found_key.type == BTRFS_IDENTITY_REMAP_KEY) {
-		btrfs_free_path(path);
+	if (found_key.type == BTRFS_IDENTITY_REMAP_KEY)
 		return 0;
-	}
 
 	remap = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_remap_item);
-	*logical = *logical - found_key.objectid + btrfs_remap_address(leaf, remap);
-
-	btrfs_free_path(path);
+	*logical += btrfs_remap_address(leaf, remap) - found_key.objectid;
 
 	return 0;
 }
-- 
2.56.0

[PATCH 4/5] btrfs-progs: fix a path leak in alloc_reserved_tree_block()

[Qu Wenruo]

If btrfs_insert_empty_item() failed with -EEXIST, the path is not
released by btrfs_insert_empty_item().

But the function returns without freeing @path, causing a leak.

This function has lagged behind kernel quite some, so here I just fix
the path leak by utilizing BTRFS_PATH_AUTO_FREE() inside the if ()
branch.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/extent-tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel-shared/extent-tree.c b/kernel-shared/extent-tree.c
index 64479e6e43f1..bac44654eab8 100644
--- a/kernel-shared/extent-tree.c
+++ b/kernel-shared/extent-tree.c
@@ -2403,7 +2403,6 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 	struct btrfs_extent_inline_ref *iref;
 	struct btrfs_space_info *sinfo;
 	struct extent_buffer *leaf;
-	struct btrfs_path *path;
 	struct btrfs_key ins;
 	u32 size = sizeof(*extent_item) + sizeof(*iref);
 	u64 start, end;
@@ -2415,6 +2414,8 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 	ins.objectid = node->bytenr;
 
 	if (ref->root != BTRFS_REMAP_TREE_OBJECTID) {
+		BTRFS_PATH_AUTO_FREE(path);
+
 		if (skinny_metadata) {
 			ins.type = BTRFS_METADATA_ITEM_KEY;
 			ins.offset = ref->level;
@@ -2462,7 +2463,6 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 		btrfs_set_extent_inline_ref_offset(leaf, iref, ref->root);
 
 		btrfs_mark_buffer_dirty(leaf);
-		btrfs_free_path(path);
 	}
 
 	ret = remove_from_free_space_tree(trans, ins.objectid, fs_info->nodesize);
-- 
2.56.0

[PATCH 5/5] btrfs-progs: fix several memory bugs in btrfs-corrupt-block

[Qu Wenruo]

These involves several call sites:

- BTRFS_METADATA_BLOCK_GENERATION branch of corrupt_metadata_block()

- BTRFS_METADATA_BLOCK_SHIFT_ITEMS branch of corrupt_metadata_block()

- if (chunk_rec) branch of main()

The bugs are:

- Use-after-free if write_data_to_disk() failed in
  BTRFS_METADATA_BLOCK_GENERATION branch
  Fix it by saving @eb->start and use that saved value for error
  message.

- Path leak in the successful path
  Fix it by using BTRFS_PATH_AUTO_FREE() and remove all existing
  btrfs_free_path().

- Path held after btrfs_commit_transaction()
  This is minor, but still release the path before
  btrfs_commit_transaction() to follow the kernel behavior.

- Missing error message and code if btrfs_commit_transaction failed
  Just add the missing error message and save the error code from it.

- Wrong value in the error message if btrfs_read_fs_root() failed
  Fix it by printing root_key.objectid.

- Missing error code if early failures happened in chunk_rec branch
  Fix it by setting @ret to -ENOMEM or -ENOENT respectively.

Behavior changes:

- Abort the transaction if corrupt_item_nocow() failed in chunk_rec
  branch
  Previously we would still commit the transaction after a failed
  corrupt_item_nocow(), which is meaningless for a failed operation.
  Now we abort the transaction and exit through out_close, without
  committing.

Cleanup:

- Remove the dead "eb = path->nodes[level]" assignment in SHIFT_ITEMS
  branch
  @eb is not used after that, and it was already freed earlier.

Assisted-by: LLM (generate the commit message with human modification)
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 btrfs-corrupt-block.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 94815221a32d..0fc2c42e7ffa 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -910,6 +910,7 @@ static int corrupt_metadata_block(struct btrfs_fs_info *fs_info, u64 block,
 		{
 		u64 orig = btrfs_header_generation(eb);
 		u64 bogus = generate_u64(orig);
+		const u64 start = eb->start;
 
 		btrfs_set_header_generation(eb, bogus);
 		csum_tree_block_size(eb, fs_info->csum_size, 0,
@@ -919,7 +920,7 @@ static int corrupt_metadata_block(struct btrfs_fs_info *fs_info, u64 block,
 		if (ret < 0) {
 			errno = -ret;
 			error("failed to write extent buffer at %llu: %m",
-				eb->start);
+			      start);
 			return ret;
 		}
 		break;
@@ -928,7 +929,7 @@ static int corrupt_metadata_block(struct btrfs_fs_info *fs_info, u64 block,
 		{
 		struct btrfs_trans_handle *trans;
 		struct btrfs_root *root;
-		struct btrfs_path *path;
+		BTRFS_PATH_AUTO_FREE(path);
 		struct btrfs_key key, root_key;
 		u64 root_objectid;
 		u8 level;
@@ -947,7 +948,7 @@ static int corrupt_metadata_block(struct btrfs_fs_info *fs_info, u64 block,
 
 		root = btrfs_read_fs_root(fs_info, &root_key);
 		if (IS_ERR(root)) {
-			error("couldn't find owner root %llu", key.objectid);
+			error("couldn't find owner root %llu", root_key.objectid);
 			return PTR_ERR(root);
 		}
 
@@ -957,7 +958,6 @@ static int corrupt_metadata_block(struct btrfs_fs_info *fs_info, u64 block,
 
 		trans = btrfs_start_transaction(root, 1);
 		if (IS_ERR(trans)) {
-			btrfs_free_path(path);
 			ret = PTR_ERR(trans);
 			errno = -ret;
 			error_msg(ERROR_MSG_START_TRANS, "%m");
@@ -968,14 +968,17 @@ static int corrupt_metadata_block(struct btrfs_fs_info *fs_info, u64 block,
 		ret = btrfs_search_slot(trans, root, &key, path, 0, 1);
 		if (ret < 0) {
 			error("error searching to node %d", ret);
-			btrfs_free_path(path);
 			btrfs_abort_transaction(trans, ret);
 			return ret;
 		}
-		eb = path->nodes[level];
 		shift_items(root, path->nodes[level]);
 		btrfs_mark_buffer_dirty(path->nodes[level]);
-		btrfs_commit_transaction(trans, root);
+		btrfs_release_path(path);
+		ret = btrfs_commit_transaction(trans, root);
+		if (ret < 0) {
+			errno = -ret;
+			error_msg(ERROR_MSG_COMMIT_TRANS, "%m");
+		}
 		break;
 		}
 	default:
@@ -1502,7 +1505,7 @@ int main(int argc, char **argv)
 	}
 	if (chunk_rec) {
 		struct btrfs_trans_handle *trans;
-		struct btrfs_path *path;
+		BTRFS_PATH_AUTO_FREE(path);
 		int del;
 
 		if (logical == (u64)-1)
@@ -1511,21 +1514,30 @@ int main(int argc, char **argv)
 		path = btrfs_alloc_path();
 		if (!path) {
 			error_mem(NULL);
+			ret = -ENOMEM;
 			goto out_close;
 		}
 
 		if (find_chunk_offset(root->fs_info->chunk_root, path,
 				      logical) != 0) {
-			btrfs_free_path(path);
+			ret = -ENOENT;
 			goto out_close;
 		}
 		trans = btrfs_start_transaction(root, 1);
 		BUG_ON(IS_ERR(trans));
 		ret = corrupt_item_nocow(trans, root->fs_info->chunk_root,
 					 path, del);
-		if (ret < 0)
+		btrfs_release_path(path);
+		if (ret < 0) {
 			error("failed to corrupt chunk record");
-		btrfs_commit_transaction(trans, root);
+			btrfs_abort_transaction(trans, ret);
+			goto out_close;
+		}
+		ret = btrfs_commit_transaction(trans, root);
+		if (ret < 0) {
+			errno = -ret;
+			error_msg(ERROR_MSG_COMMIT_TRANS, "%m");
+		}
 		goto out_close;
 	}
 	if (chunk_tree) {
-- 
2.56.0