[PATCH 1/7] e2fsck: value stored to err is never read

[PATCH 1/7] e2fsck: value stored to err is never read

[Lukas Czerner]

Remove it to silence clang warning.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/recovery.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/e2fsck/recovery.c b/e2fsck/recovery.c
index 25744f08..48b5efd2 100644
--- a/e2fsck/recovery.c
+++ b/e2fsck/recovery.c
@@ -760,7 +760,6 @@ static int do_one_pass(journal_t *journal,
 				 */
 				jbd_debug(1, "JBD2: Invalid checksum ignored in transaction %u, likely stale data\n",
 					  next_commit_ID);
-				err = 0;
 				brelse(bh);
 				goto done;
 			}
-- 
2.31.1

[PATCH 2/7] ext2fs: initialize retval before using it

[Lukas Czerner]

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ext2fs/gen_bitmap64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ext2fs/gen_bitmap64.c b/lib/ext2fs/gen_bitmap64.c
index a2b89898..d9809084 100644
--- a/lib/ext2fs/gen_bitmap64.c
+++ b/lib/ext2fs/gen_bitmap64.c
@@ -948,7 +948,7 @@ errcode_t ext2fs_count_used_clusters(ext2_filsys fs, blk64_t start,
 {
 	blk64_t		next;
 	blk64_t		tot_set = 0;
-	errcode_t	retval;
+	errcode_t	retval = 0;
 
 	while (start < end) {
 		retval = ext2fs_find_first_set_block_bitmap2(fs->block_map,
-- 
2.31.1

Re: [PATCH 2/7] ext2fs: initialize retval before using it

[Theodore Ts'o]

On Fri, Aug 06, 2021 at 11:58:15AM +0200, Lukas Czerner wrote:
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied, thanks.

					- Ted

[PATCH 3/7] e2fsprogs: fix unexpected NULL variable

[Lukas Czerner]

The ext2fs_check_mount_point() function can be called with mtpt being
NULL as for example from ext2fs_check_if_mounted(). However in the
is_swap_device condition we use the mtpt in strncpy without checking
whether it is non-null first.

This should not be a problem on linux since the previous attempt to open
the device exclusively would have prevented us from ever reaching the
problematic strncpy. However it's still a bug and can cause problems on
other systems, fix it by conditioning strncpy on mtpt not being null.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ext2fs/ismounted.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c
index c9e6a9d0..aee7d726 100644
--- a/lib/ext2fs/ismounted.c
+++ b/lib/ext2fs/ismounted.c
@@ -393,7 +393,8 @@ errcode_t ext2fs_check_mount_point(const char *device, int *mount_flags,
 
 	if (is_swap_device(device)) {
 		*mount_flags = EXT2_MF_MOUNTED | EXT2_MF_SWAP;
-		strncpy(mtpt, "<swap>", mtlen);
+		if (mtpt)
+			strncpy(mtpt, "<swap>", mtlen);
 	} else {
 #ifdef HAVE_SETMNTENT
 		retval = check_mntent(device, mount_flags, mtpt, mtlen);
-- 
2.31.1

Re: [PATCH 3/7] e2fsprogs: fix unexpected NULL variable

[Theodore Ts'o]

On Fri, Aug 06, 2021 at 11:58:16AM +0200, Lukas Czerner wrote:
> The ext2fs_check_mount_point() function can be called with mtpt being
> NULL as for example from ext2fs_check_if_mounted(). However in the
> is_swap_device condition we use the mtpt in strncpy without checking
> whether it is non-null first.
> 
> This should not be a problem on linux since the previous attempt to open
> the device exclusively would have prevented us from ever reaching the
> problematic strncpy. However it's still a bug and can cause problems on
> other systems, fix it by conditioning strncpy on mtpt not being null.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied, thanks.

					- Ted

[PATCH 4/7] e2fsprogs: remove augmented rbtree functionality

[Lukas Czerner]

Rbtree code was originally taken from linux kernel. This includes the
augmented rbtree functionality, however this was never intended to be
used and is not used still. Just remove it.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ext2fs/rbtree.c | 68 ---------------------------------------------
 lib/ext2fs/rbtree.h |  8 ------
 2 files changed, 76 deletions(-)

diff --git a/lib/ext2fs/rbtree.c b/lib/ext2fs/rbtree.c
index 5b92099d..74426fa6 100644
--- a/lib/ext2fs/rbtree.c
+++ b/lib/ext2fs/rbtree.c
@@ -280,74 +280,6 @@ void ext2fs_rb_erase(struct rb_node *node, struct rb_root *root)
 		__rb_erase_color(child, parent, root);
 }
 
-static void ext2fs_rb_augment_path(struct rb_node *node, rb_augment_f func, void *data)
-{
-	struct rb_node *parent;
-
-up:
-	func(node, data);
-	parent = ext2fs_rb_parent(node);
-	if (!parent)
-		return;
-
-	if (node == parent->rb_left && parent->rb_right)
-		func(parent->rb_right, data);
-	else if (parent->rb_left)
-		func(parent->rb_left, data);
-
-	node = parent;
-	goto up;
-}
-
-/*
- * after inserting @node into the tree, update the tree to account for
- * both the new entry and any damage done by rebalance
- */
-void ext2fs_rb_augment_insert(struct rb_node *node, rb_augment_f func, void *data)
-{
-	if (node->rb_left)
-		node = node->rb_left;
-	else if (node->rb_right)
-		node = node->rb_right;
-
-	ext2fs_rb_augment_path(node, func, data);
-}
-
-/*
- * before removing the node, find the deepest node on the rebalance path
- * that will still be there after @node gets removed
- */
-struct rb_node *ext2fs_rb_augment_erase_begin(struct rb_node *node)
-{
-	struct rb_node *deepest;
-
-	if (!node->rb_right && !node->rb_left)
-		deepest = ext2fs_rb_parent(node);
-	else if (!node->rb_right)
-		deepest = node->rb_left;
-	else if (!node->rb_left)
-		deepest = node->rb_right;
-	else {
-		deepest = ext2fs_rb_next(node);
-		if (deepest->rb_right)
-			deepest = deepest->rb_right;
-		else if (ext2fs_rb_parent(deepest) != node)
-			deepest = ext2fs_rb_parent(deepest);
-	}
-
-	return deepest;
-}
-
-/*
- * after removal, update the tree to account for the removed entry
- * and any rebalance damage.
- */
-void ext2fs_rb_augment_erase_end(struct rb_node *node, rb_augment_f func, void *data)
-{
-	if (node)
-		ext2fs_rb_augment_path(node, func, data);
-}
-
 /*
  * This function returns the first node (in sort order) of the tree.
  */
diff --git a/lib/ext2fs/rbtree.h b/lib/ext2fs/rbtree.h
index dfeeb234..f718ad24 100644
--- a/lib/ext2fs/rbtree.h
+++ b/lib/ext2fs/rbtree.h
@@ -151,14 +151,6 @@ static inline void ext2fs_rb_clear_node(struct rb_node *node)
 extern void ext2fs_rb_insert_color(struct rb_node *, struct rb_root *);
 extern void ext2fs_rb_erase(struct rb_node *, struct rb_root *);
 
-typedef void (*rb_augment_f)(struct rb_node *node, void *data);
-
-extern void ext2fs_rb_augment_insert(struct rb_node *node,
-			      rb_augment_f func, void *data);
-extern struct rb_node *ext2fs_rb_augment_erase_begin(struct rb_node *node);
-extern void ext2fs_rb_augment_erase_end(struct rb_node *node,
-				 rb_augment_f func, void *data);
-
 /* Find logical next and previous nodes in a tree */
 extern struct rb_node *ext2fs_rb_next(struct rb_node *);
 extern struct rb_node *ext2fs_rb_prev(struct rb_node *);
-- 
2.31.1

Re: [PATCH 4/7] e2fsprogs: remove augmented rbtree functionality

[Theodore Ts'o]

On Fri, Aug 06, 2021 at 11:58:17AM +0200, Lukas Czerner wrote:
> Rbtree code was originally taken from linux kernel. This includes the
> augmented rbtree functionality, however this was never intended to be
> used and is not used still. Just remove it.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied, thanks.

					- Ted

[PATCH 5/7] libss: handle memory allcation failure in ss_help()

[Lukas Czerner]

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ss/help.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/ss/help.c b/lib/ss/help.c
index 96eb1092..a22b4017 100644
--- a/lib/ss/help.c
+++ b/lib/ss/help.c
@@ -96,7 +96,12 @@ void ss_help(int argc, char const * const *argv, int sci_idx, pointer info_ptr)
     }
     if (fd < 0) {
 #define MSG "No info found for "
-        char *buf = malloc(strlen (MSG) + strlen (argv[1]) + 1);
+	char *buf = malloc(strlen (MSG) + strlen (argv[1]) + 1);
+	if (!buf) {
+		ss_perror(sci_idx, 0,
+			  "couldn't allocate memory to print error message");
+		return;
+	}
 	strcpy(buf, MSG);
 	strcat(buf, argv[1]);
 	ss_perror(sci_idx, 0, buf);
-- 
2.31.1

Re: [PATCH 5/7] libss: handle memory allcation failure in ss_help()

[Theodore Ts'o]

On Fri, Aug 06, 2021 at 11:58:18AM +0200, Lukas Czerner wrote:
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Thanks, applied.

					- Ted

[PATCH 6/7] libss: Add missing error handling for fdopen()

[Lukas Czerner]

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ss/list_rqs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/ss/list_rqs.c b/lib/ss/list_rqs.c
index 021a3835..89e37bb2 100644
--- a/lib/ss/list_rqs.c
+++ b/lib/ss/list_rqs.c
@@ -12,6 +12,9 @@
  */
 #include "config.h"
 #include "ss_internal.h"
+#ifdef HAVE_UNISTD_H
+#include <unistd.h>
+#endif
 #include <signal.h>
 #include <setjmp.h>
 #include <sys/wait.h>
@@ -46,6 +49,12 @@ void ss_list_requests(int argc __SS_ATTR((unused)),
         return;
     }
     output = fdopen(fd, "w");
+    if (!output) {
+        perror("fdopen");
+        close(fd);
+        (void) signal(SIGINT, func);
+        return;
+    }
     sigprocmask(SIG_SETMASK, &omask, (sigset_t *) 0);
 
     fprintf (output, "Available %s requests:\n\n",
-- 
2.31.1

Re: [PATCH 6/7] libss: Add missing error handling for fdopen()

[Theodore Ts'o]

On Fri, Aug 06, 2021 at 11:58:19AM +0200, Lukas Czerner wrote:
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Thanks, applied.

					- Ted

[PATCH 7/7] mkquota: Fix potental NULL pointer dereference

[Lukas Czerner]

get_dq() function can fail when the memory allocation fails and so we
could end up dereferencing NULL pointer. Fix it.

Also, we should really return -ENOMEM instead of -1, or even 0 from
various functions in quotaio_tree.c when memory allocation fails.
Fix it as well.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/support/mkquota.c      | 8 ++++++--
 lib/support/quotaio_tree.c | 8 ++++----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/support/mkquota.c b/lib/support/mkquota.c
index dce077e6..420ba503 100644
--- a/lib/support/mkquota.c
+++ b/lib/support/mkquota.c
@@ -433,7 +433,8 @@ void quota_data_sub(quota_ctx_t qctx, struct ext2_inode_large *inode,
 		dict = qctx->quota_dict[qtype];
 		if (dict) {
 			dq = get_dq(dict, get_qid(inode, qtype));
-			dq->dq_dqb.dqb_curspace -= space;
+			if (dq)
+				dq->dq_dqb.dqb_curspace -= space;
 		}
 	}
 }
@@ -460,7 +461,8 @@ void quota_data_inodes(quota_ctx_t qctx, struct ext2_inode_large *inode,
 		dict = qctx->quota_dict[qtype];
 		if (dict) {
 			dq = get_dq(dict, get_qid(inode, qtype));
-			dq->dq_dqb.dqb_curinodes += adjust;
+			if (dq)
+				dq->dq_dqb.dqb_curinodes += adjust;
 		}
 	}
 }
@@ -533,6 +535,8 @@ static int scan_dquots_callback(struct dquot *dquot, void *cb_data)
 	struct dquot *dq;
 
 	dq = get_dq(quota_dict, dquot->dq_id);
+	if (!dq)
+		return -ENOMEM;
 	dq->dq_id = dquot->dq_id;
 	dq->dq_flags |= DQF_SEEN;
 
diff --git a/lib/support/quotaio_tree.c b/lib/support/quotaio_tree.c
index 6cc4fb5b..65e68792 100644
--- a/lib/support/quotaio_tree.c
+++ b/lib/support/quotaio_tree.c
@@ -569,7 +569,7 @@ static int report_block(struct dquot *dquot, unsigned int blk, char *bitmap,
 	int entries, i;
 
 	if (!buf)
-		return -1;
+		return -ENOMEM;
 
 	set_bit(bitmap, blk);
 	read_blk(dquot->dq_h, blk, buf);
@@ -601,7 +601,7 @@ static int report_tree(struct dquot *dquot, unsigned int blk, int depth,
 	__le32 *ref = (__le32 *) buf;
 
 	if (!buf)
-		return 0;
+		return -ENOMEM;
 
 	read_blk(dquot->dq_h, blk, buf);
 	if (depth == QT_TREEDEPTH - 1) {
@@ -667,12 +667,12 @@ int qtree_scan_dquots(struct quota_handle *h,
 	struct dquot *dquot = get_empty_dquot();
 
 	if (!dquot)
-		return -1;
+		return -ENOMEM;
 
 	dquot->dq_h = h;
 	if (ext2fs_get_memzero((info->dqi_blocks + 7) >> 3, &bitmap)) {
 		ext2fs_free_mem(&dquot);
-		return -1;
+		return -ENOMEM;
 	}
 	ret = report_tree(dquot, QT_TREEOFF, 0, bitmap, process_dquot, data);
 	if (ret < 0)
-- 
2.31.1

Re: [PATCH 7/7] mkquota: Fix potental NULL pointer dereference

[Theodore Ts'o]

On Fri, Aug 06, 2021 at 11:58:20AM +0200, Lukas Czerner wrote:
> get_dq() function can fail when the memory allocation fails and so we
> could end up dereferencing NULL pointer. Fix it.
> 
> Also, we should really return -ENOMEM instead of -1, or even 0 from
> various functions in quotaio_tree.c when memory allocation fails.
> Fix it as well.

The quota*.c files were taking from the quota_tools package, and are
currently using the converion of setting errno and returning -1.  I
don't think an incomplete conversion to the kernel error return
convention is the way to go.  My long term plan for the quota
functions in libsupport is to convert them to use the comerr_t error
return convention, remove all of the printf functions from the
functions, so they can be properly moved into libext2fs library as a
first class supported library functions, and so that the high-level
ext2fs functions would update the quota files --- so that programs
like fuse2fs would properly update the quota records.

So I'm going to drop the error handling changes from this patch before
applying it.

Cheers,

					- Ted

Re: [PATCH 7/7] mkquota: Fix potental NULL pointer dereference

[Lukas Czerner]

On Tue, Aug 10, 2021 at 12:15:28PM -0400, Theodore Ts'o wrote:
> On Fri, Aug 06, 2021 at 11:58:20AM +0200, Lukas Czerner wrote:
> > get_dq() function can fail when the memory allocation fails and so we
> > could end up dereferencing NULL pointer. Fix it.
> > 
> > Also, we should really return -ENOMEM instead of -1, or even 0 from
> > various functions in quotaio_tree.c when memory allocation fails.
> > Fix it as well.
> 
> The quota*.c files were taking from the quota_tools package, and are
> currently using the converion of setting errno and returning -1.  I
> don't think an incomplete conversion to the kernel error return
> convention is the way to go.  My long term plan for the quota
> functions in libsupport is to convert them to use the comerr_t error
> return convention, remove all of the printf functions from the
> functions, so they can be properly moved into libext2fs library as a
> first class supported library functions, and so that the high-level
> ext2fs functions would update the quota files --- so that programs
> like fuse2fs would properly update the quota records.
> 
> So I'm going to drop the error handling changes from this patch before
> applying it.

Understood, thanks!

-Lukas

> 
> Cheers,
> 
> 					- Ted
> 

Re: [PATCH 1/7] e2fsck: value stored to err is never read

[Theodore Ts'o]

On Fri, Aug 06, 2021 at 11:58:14AM +0200, Lukas Czerner wrote:
> Remove it to silence clang warning.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied, thanks.

Note that we try to keep e2fsck/recovery.c and fs/jbd2/recovery.c in
sync, so it's appreciated patches sent to e2fsck/recovery.c or
fs/jbd2/recovery.c is sent to the other.  I can take care of it in
this case.

Cheers,

					- Ted

Re: [PATCH 1/7] e2fsck: value stored to err is never read

[Lukas Czerner]

On Tue, Aug 10, 2021 at 10:58:56AM -0400, Theodore Ts'o wrote:
> On Fri, Aug 06, 2021 at 11:58:14AM +0200, Lukas Czerner wrote:
> > Remove it to silence clang warning.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> Applied, thanks.
> 
> Note that we try to keep e2fsck/recovery.c and fs/jbd2/recovery.c in
> sync, so it's appreciated patches sent to e2fsck/recovery.c or
> fs/jbd2/recovery.c is sent to the other.  I can take care of it in
> this case.
> 
> Cheers,
> 
> 					- Ted

Allright, I'll keep that in mind for the next time.

Thanks!
-Lukas