[PATCH v2] erofs: Prevent entering an infinite loop when i is 0

[Sandeep Dhavale]

On Wed, Sep 4, 2024 at 7:56 PM Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
>
> On 2024/8/31 10:58, Gao Xiang wrote:
> > Hi Sandeep,
> >
> > On 2024/8/31 05:46, Sandeep Dhavale wrote:
> >> Hi Liujinbao,
> >> On Thu, Aug 29, 2024 at 5:24 AM liujinbao1 <jinbaoliu365 at gmail.com> wrote:
> >>>
> >>> From: liujinbao1 <liujinbao1 at xiaomi.com>
> >>>
> >>> When i=0 and err is not equal to 0,
> >>> the while(-1) loop will enter into an
> >>> infinite loop. This patch avoids this issue
> >>>
> >>> Signed-off-by: liujinbao1 <liujinbao1 at xiaomi.com>
> >>> ---
> >>>   fs/erofs/decompressor.c | 10 +++++-----
> >>>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> >>> index c2253b6a5416..672f097966fa 100644
> >>> --- a/fs/erofs/decompressor.c
> >>> +++ b/fs/erofs/decompressor.c
> >>> @@ -534,18 +534,18 @@ int z_erofs_parse_cfgs(struct super_block *sb, struct erofs_super_block *dsb)
> >>>
> >>>   int __init z_erofs_init_decompressor(void)
> >>>   {
> >>> -       int i, err;
> >>> +       int i, err = 0;
> >>>
> >>>          for (i = 0; i < Z_EROFS_COMPRESSION_MAX; ++i) {
> >>>                  err = z_erofs_decomp[i] ? z_erofs_decomp[i]->init() : 0;
> >>> -               if (err) {
> >>> -                       while (--i)
> >>> +               if (err && i) {
> >>> +                       while (i--)
> >> Actually there is a subtle bug in this fix. We will never enter the if
> >> block here when i=0 and err is set which we were trying to fix.
> >> This will cause z_erofs_decomp[0]->init() error to get masked and we
> >> will continue the outer for loop (i.e. when i=0 and err is set).
> >
>
Hi Gao,
> Ping? could anyone submit a proper fix for this?
>
> Yes, that needs to be resolved, and I will replace the patch to the new version.
I think I misunderstood this as you are amending it.

> Or just
>
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index c2253b6a5416..dfb77f4e68b4 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -539,8 +539,8 @@ int __init z_erofs_init_decompressor(void)
>          for (i = 0; i < Z_EROFS_COMPRESSION_MAX; ++i) {
>                  err = z_erofs_decomp[i] ? z_erofs_decomp[i]->init() : 0;
>                  if (err) {
> -                       while (--i)
> -                               if (z_erofs_decomp[i])
> +                       while (i)
> +                               if (z_erofs_decomp[--i])
>                                          z_erofs_decomp[i]->exit();
Even though logically same, decrementing `i` in the middle of 3 lines
referencing it does not feel very readable to me.
A below version is more traditional as changing the cond in the while() check.

diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index c2253b6a5416..eb318c7ddd80 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -539,7 +539,7 @@ int __init z_erofs_init_decompressor(void)
        for (i = 0; i < Z_EROFS_COMPRESSION_MAX; ++i) {
                err = z_erofs_decomp[i] ? z_erofs_decomp[i]->init() : 0;
                if (err) {
-                       while (--i)
+                       while (i--)
                                if (z_erofs_decomp[i])
                                        z_erofs_decomp[i]->exit();
                        return err;

I will send this in a formal patch in few mins as v3, if you feel ok,
please add it. If you want to stick to your version, no problem, I
understand that it is subjective.

Thanks,
Sandeep.

>                          return err;
>                  }
>
> to avoid underflowed `i` (although it should have no real impact.)
>
> Thanks,
> Gao Xiang