Hi Richard,
Le 24/06/2026 à 17:23, Richard GENOUD a écrit :
> Having a 2nd look at the whole function, it seems that there a lot of:
> ret = -Exxx;
> goto out;
> without freeing dirs->entry
You're right, my v2 only fixed one of them. Going through the function
again, the entry is also leaked on the -ELOOP path, on all seven
allocation/tokenization failures in the symlink resolution block, and
in a more subtle case: when sqfs_readdir_nest() aborts via its
sqfs_find_inode() failure path *after* sqfs_read_entry() succeeded,
the while loop exits with a live entry that the "Cannot find
directory" path then leaks.
> It seems to me that we could keep free(dirs->entry):
> - in the while (!sqfs_readdir_nest(dirsp, &dent)) loop
> - at the end of the for (j = 0; j < token_count; j++) loop
> - and before the recursive call to sqfs_search_dir()
>
> And remove all the rest to add something like:
> [...]
> out:
> + if (ret < 0) {
> + free(dirs->entry);
> + dirs->entry = NULL;
> + }
>
> (not tested)
>
> What do you think?
I agree, this is better than fixing each site individually. I checked
that it holds on every path:
- On success (ret == 0), dirs->entry is provably already NULL: it is
freed at the end of each token iteration and before recursing into
a symlink target, and the root-directory early return never
allocates it. So the conditional free at out: can never take away
an entry a caller would use (sqfs_opendir_nest() doesn't read it
after return anyway).
- No double free is possible: every free site resets the pointer to
NULL, including the recursive case, where the child's out: label
runs before the parent's.
- The three frees you list are exactly the ones that must stay: the
first two run on success paths that the "ret < 0" guard at out:
would skip, and the one before the recursive call is needed because
the child's sqfs_read_entry() overwrites the pointer without
freeing it.
- SQFS_EMPTY_DIR is -1, so the empty-directory path is covered by the
"ret < 0" test like the current explicit free.
I've reworked patch 3 accordingly for v3 (with your Suggested-by),
dropping the now-redundant frees on the individual error paths,
including the two added by patch 2. Tested on sandbox with the
squashfs test suite ("test.py -k sqfs") plus a manual sqfsload
through a symlink on the image that originally triggered the bug
report, and the error paths (missing file, bad directory).
v3 also moves the collected tags before my Signed-off-by, per your
remark on patch 2. Thanks for the review!
Regards,
Allan
Le mer. 24 juin 2026 à 17:24, Richard GENOUD <[email protected]> a
écrit :
> Le 11/06/2026 à 11:43, Allan ELKAIM a écrit :
> > In sqfs_search_dir(), when sqfs_find_inode() fails to locate the inode
> > of a directory entry just returned by sqfs_readdir_nest(), the function
> > returns directly while dirs->entry still holds the entry allocation,
> > leaking it. The bare return also bypasses the regular error path.
> >
> > Free the entry and leave through the out label instead, consistent
> > with the other error paths in this function.
> >
> > Signed-off-by: Allan ELKAIM <[email protected]>
> > ---
> >
> > Changes in v2:
> > - New patch, fixing a pre-existing leak of the same kind as the ones
> > addressed in patch 2 (suggested by Richard Genoud's review)
> >
> > fs/squashfs/sqfs.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> > index af32d008..df988774 100644
> > --- a/fs/squashfs/sqfs.c
> > +++ b/fs/squashfs/sqfs.c
> > @@ -547,8 +547,12 @@ static int sqfs_search_dir(struct
> squashfs_dir_stream *dirs, char **token_list,
> > /* Get reference to inode in the inode table */
> > table = sqfs_find_inode(dirs->inode_table,
> new_inode_number,
> > sblk->inodes, sblk->block_size);
> > - if (!table)
> > - return -EINVAL;
> > + if (!table) {
> > + free(dirs->entry);
> > + dirs->entry = NULL;
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > dir = (struct squashfs_dir_inode *)table;
> >
> > /* Check for symbolic link and inode type sanity */
>
> Having a 2nd look at the whole function, it seems that there a lot of:
> ret = -Exxx;
> goto out;
> without freeing dirs->entry
>
> Like here:
> [...]
> if (++symlinknest == MAX_SYMLINK_NEST) {
> ret = -ELOOP;
> goto out;
> }
>
> sym = (struct squashfs_symlink_inode *)table;
> /* Get first j + 1 tokens */
> path = sqfs_concat_tokens(token_list, j + 1);
> if (!path) {
> ret = -ENOMEM;
> goto out;
> }
> /* Resolve for these tokens */
> target = sqfs_resolve_symlink(sym, path);
> if (!target) {
> ret = -ENOMEM;
> goto out;
> }
> [...]
>
> It seems to me that we could keep free(dirs->entry):
> - in the while (!sqfs_readdir_nest(dirsp, &dent)) loop
> - at the end of the for (j = 0; j < token_count; j++) loop
> - and before the recursive call to sqfs_search_dir()
>
> And remove all the rest to add something like:
> @@ -677,6 +677,10 @@ static int sqfs_search_dir(struct squashfs_dir_stream
> *dirs, char **token_list,
> memcpy(&dirs->i_ldir, ldir, sizeof(*ldir));
>
> out:
> + if (ret < 0) {
> + free(dirs->entry);
> + dirs->entry = NULL;
> + }
> free(res);
> free(rem);
> free(path);
>
> (not tested)
>
> What do you think?
>
>
> Regards,
> Richard
>
