回复: lib/fts.c: return when malloc failed

[ChuanGang Jiang]

I found this by accident and then reproduce it through artificial mem pressure 
test.
And I update the patch as you said.

*lib/fts.c：return when malloc failed in function setup_dir()
--- 
 lib/fts.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/fts.c b/lib/fts.c
index 78584b2902..c2fa5ee8dc 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -979,7 +979,11 @@ next:   tmp = p;
                         }
                         free_dir(sp);
                         fts_load(sp, p);
-                        setup_dir(sp);
+                        if (! setup_dir(sp)) {
+                                free_dir(sp);
+                                __set_errno (ENOMEM);
+                                return (NULL);
+                        }
                         goto check_for_dir;
                 }

--
2.36.1

Best Regards
ChuanGang Jiang

-----邮件原件-----
发件人: Pádraig Brady <pixelbeat@gmail.com> 代表 Pádraig Brady
发送时间: 2023年2月26日 23:30
收件人: ChuanGang Jiang <jiangchuanganghw@outlook.com>; bug-gnulib@gnu.org
主题: Re: lib/fts.c: return when malloc failed

On 26/02/2023 14:46, ChuanGang Jiang wrote:
> Hi，‘rm’ of coreutils was terminated with signal SIGSEGV, Segmentation fault.
> Here is the backtrace with gdb.
> 
> Core was generated by `/usr/bin/rm -rf test1/test2/'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  cycle_check (state=0x0, sb=sb@entry=0x56296ac29e00) at 
> ../lib/cycle-check.c:60
> 60        assure (state->magic == CC_MAGIC);
> (gdb) bt full
> #0  cycle_check (state=0x0, sb=sb@entry=0x56296ac29e00) at 
> ../lib/cycle-check.c:60
>          __PRETTY_FUNCTION__ = "cycle_check"
> #1  0x000056296aa3d8bd in enter_dir (fts=0x56296ac28ae0, ent=0x56296ac29d90) 
> at ../lib/fts-cycle.c:108
>          st = <optimized out>
>          ad = <optimized out>
>          ad_from_table = <optimized out>
> #2  0x000056296aa3f031 in rpl_fts_read (sp=sp@entry=0x56296ac28ae0) at 
> ../lib/fts.c:1024
>          p = 0x56296ac29d90
>          tmp = <optimized out>
>          instr = <optimized out>
>          t = <optimized out>
> #3  0x000056296aa39858 in rm (file=<optimized out>, x=0x7ffc10c71c60) at 
> ../src/remove.c:597
>          ent = <optimized out>
>          s = <optimized out>
>          bit_flags = <optimized out>
>          fts = 0x56296ac28ae0
>          rm_status = RM_OK
>          __PRETTY_FUNCTION__ = "rm"
> #4  0x000056296aa388e2 in main (argc=<optimized out>, argv=0x7ffc10c71e88) at 
> ../src/rm.c:370
>          preserve_root = true
>          x = {ignore_missing_files = true, interactive = RMI_NEVER, 
> one_file_system = false, recursive = true, remove_empty_directories = false, 
> root_dev_ino = 0x56296aa480b0 <dev_ino_buf>, preserve_all_root = false,
>            stdin_tty = true, verbose = false, require_restore_cwd = false}
>          prompt_once = <optimized out>
>          c = <optimized out>
>          n_files = <optimized out>
>          file = 0x7ffc10c71e98
>          status = <optimized out>
>          __PRETTY_FUNCTION__ = "main"
> 
> 
> I think it should return when malloc failed for ’fts->fts_cycle.state‘ 
> in ’setup_dir(sp)‘ and the patch below may fix this.
> 
> 
> *lib/fts.c：return when malloc failed in function setup_dir()
> ---
>   lib/fts.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/fts.c b/lib/fts.c
> index 78584b2902..374efa1be7 100644
> --- a/lib/fts.c
> +++ b/lib/fts.c
> @@ -979,7 +979,10 @@ next:   tmp = p;
>                           }
>                           free_dir(sp);
>                           fts_load(sp, p);
> -                        setup_dir(sp);
> +                        if (! setup_dir(sp)) {
> +                                free_dir(sp);
> +                                return (NULL);
> +                        }
>                           goto check_for_dir;
>                   }
> 

I agree that assertion may trigger if setup_dir() fails.
free_dir() is safe to call upon setup_dir() failure, so the patch looks correct.
The only way that setup_dir() can really fail is due to no memory, so I'd also 
__set_errno (ENOMEM); in this case.

How did you notice this?
Did you apply artificial mem pressure for testing?

thanks!
Pádraig