[PATCH v2] erofs-utils: lib: fix erofs_iterate_dir() recursion

[PATCH v2] erofs-utils: lib: fix erofs_iterate_dir() recursion

[Jingbo Xu]

ctx->dir may have changed when ctx is reused along erofs_iterate_dir()
recursion.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
changes since last version:
since traverse_dirents() can be called multiple times in one single
erofs_iterate_dir() call, ctx->dir may have changed at the entry of
traverse_dirents().  The previous v1 shall be deprecated.

v1: https://lore.kernel.org/all/20230718052101.124039-3-jefflexu@linux.alibaba.com/
---
 lib/dir.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/dir.c b/lib/dir.c
index abbf27a..535204b 100644
--- a/lib/dir.c
+++ b/lib/dir.c
@@ -5,6 +5,7 @@
 #include "erofs/dir.h"
 
 static int traverse_dirents(struct erofs_dir_context *ctx,
+			    struct erofs_inode *dir,
 			    void *dentry_blk, unsigned int lblk,
 			    unsigned int next_nameoff, unsigned int maxsize,
 			    bool fsck)
@@ -76,7 +77,7 @@ static int traverse_dirents(struct erofs_dir_context *ctx,
 					goto out;
 				}
 				ctx->flags |= EROFS_READDIR_DOTDOT_FOUND;
-				if (sbi.root_nid == ctx->dir->nid) {
+				if (sbi.root_nid == dir->nid) {
 					ctx->pnid = sbi.root_nid;
 					ctx->flags |= EROFS_READDIR_VALID_PNID;
 				}
@@ -95,7 +96,7 @@ static int traverse_dirents(struct erofs_dir_context *ctx,
 				}
 
 				ctx->flags |= EROFS_READDIR_DOT_FOUND;
-				if (fsck && ctx->de_nid != ctx->dir->nid) {
+				if (fsck && ctx->de_nid != dir->nid) {
 					errmsg = "corrupted `.' dirent";
 					goto out;
 				}
@@ -115,7 +116,7 @@ static int traverse_dirents(struct erofs_dir_context *ctx,
 out:
 	if (ret && !silent)
 		erofs_err("%s @ nid %llu, lblk %u, index %lu",
-			  errmsg, ctx->dir->nid | 0ULL, lblk,
+			  errmsg, dir->nid | 0ULL, lblk,
 			  (de - (struct erofs_dirent *)dentry_blk) | 0UL);
 	return ret;
 }
@@ -153,7 +154,7 @@ int erofs_iterate_dir(struct erofs_dir_context *ctx, bool fsck)
 				  nameoff, dir->nid | 0ULL, lblk);
 			return -EFSCORRUPTED;
 		}
-		err = traverse_dirents(ctx, buf, lblk, nameoff, maxsize, fsck);
+		err = traverse_dirents(ctx, dir, buf, lblk, nameoff, maxsize, fsck);
 		if (err)
 			break;
 		pos += maxsize;
-- 
1.8.3.1

Re: [PATCH v2] erofs-utils: lib: fix erofs_iterate_dir() recursion

[Gao Xiang]

Hi Jingbo,

On Sat, Jul 22, 2023 at 01:40:09PM +0800, Jingbo Xu wrote:
> ctx->dir may have changed when ctx is reused along erofs_iterate_dir()
> recursion.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
> changes since last version:
> since traverse_dirents() can be called multiple times in one single
> erofs_iterate_dir() call, ctx->dir may have changed at the entry of
> traverse_dirents().  The previous v1 shall be deprecated.
> 
> v1: https://lore.kernel.org/all/20230718052101.124039-3-jefflexu@linux.alibaba.com/

I plan to drop this commit directly.  `struct erofs_dir_context` is not
designed for reusing recursively. It's not the case just due to
`ctx->dir` but also internal states.

You need to build another ctx for recursion.

Thanks,
Gao Xiang

Re: [PATCH v2] erofs-utils: lib: fix erofs_iterate_dir() recursion

[Jingbo Xu]

On 7/22/23 2:20 PM, Gao Xiang wrote:
> Hi Jingbo,
> 
> On Sat, Jul 22, 2023 at 01:40:09PM +0800, Jingbo Xu wrote:
>> ctx->dir may have changed when ctx is reused along erofs_iterate_dir()
>> recursion.
>>
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>> ---
>> changes since last version:
>> since traverse_dirents() can be called multiple times in one single
>> erofs_iterate_dir() call, ctx->dir may have changed at the entry of
>> traverse_dirents().  The previous v1 shall be deprecated.
>>
>> v1: https://lore.kernel.org/all/20230718052101.124039-3-jefflexu@linux.alibaba.com/
> 
> I plan to drop this commit directly.  `struct erofs_dir_context` is not
> designed for reusing recursively. It's not the case just due to
> `ctx->dir` but also internal states.
> 
> You need to build another ctx for recursion.

It seems that ctx is reused to avoid stack overflow.  So we have to
allocate ctx on heap to avoid stack overflow?

-- 
Thanks,
Jingbo

Re: [PATCH v2] erofs-utils: lib: fix erofs_iterate_dir() recursion

[Gao Xiang]

On Sat, Jul 22, 2023 at 03:04:23PM +0800, Jingbo Xu wrote:
> 
> 
> On 7/22/23 2:20 PM, Gao Xiang wrote:
> > Hi Jingbo,
> > 
> > On Sat, Jul 22, 2023 at 01:40:09PM +0800, Jingbo Xu wrote:
> >> ctx->dir may have changed when ctx is reused along erofs_iterate_dir()
> >> recursion.
> >>
> >> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> >> ---
> >> changes since last version:
> >> since traverse_dirents() can be called multiple times in one single
> >> erofs_iterate_dir() call, ctx->dir may have changed at the entry of
> >> traverse_dirents().  The previous v1 shall be deprecated.
> >>
> >> v1: https://lore.kernel.org/all/20230718052101.124039-3-jefflexu@linux.alibaba.com/
> > 
> > I plan to drop this commit directly.  `struct erofs_dir_context` is not
> > designed for reusing recursively. It's not the case just due to
> > `ctx->dir` but also internal states.
> > 
> > You need to build another ctx for recursion.
> 
> It seems that ctx is reused to avoid stack overflow.  So we have to
> allocate ctx on heap to avoid stack overflow?

It'd be better to use heap space for allocation in principle (even
avoid recursion), but I think it's just some userspace code and
stack space to enough in general.

Thanks,
Gao Xiang

> 
> -- 
> Thanks,
> Jingbo