Re: Crash in gconv_db.c

[Abhidnya Joshi <abhidnyachirmule@gmail.com>]

Hi Florian,

In increment_counter function, do we have to handle integer overflow like this?


if (step->__counter == INT_MAX)
{
step->__counter = 1
}
if (step->__counter++ == 0)
{ ....
}

I do not understand what will be impact of this on different "steps".
If step->__counter gets set to 0 and step->__modname is NULL
then we see the segfault as mentioned earlier.

Can you please suggest a way out to check integer overflow along with
other checks which are necessary to consider?

Thanks
Abhidnya

On Tue, Nov 19, 2019 at 7:35 PM Abhidnya Joshi
<abhidnyachirmule@gmail.com> wrote:
>
> I tried to experiment again. When I run conversions via readdir, I can
> see counter increasing as below:
>
> (gdb) c
> Continuing.
> [Switching to Thread 0x7f936d472700 (LWP 16095)]
>
> Breakpoint 1, find_derivation (toset=toset@entry=0x7f936d46e990
> "UTF-16LE//", toset_expand=0x0, fromset=fromset@entry=0x7f936d46e970
> "UTF-8//",
>     fromset_expand=fromset_expand@entry=0x7f9392c17ae8
> "ISO-10646/UTF8/", handle=handle@entry=0x7f936d46e900,
> nsteps=nsteps@entry=0x7f936d46e910) at gconv_db.c:426
> 426     gconv_db.c: No such file or directory.
> (gdb) s
> 0x00007f93948e62be in increment_counter (nsteps=2,
> steps=0x7f9392c18500) at gconv_db.c:410
> 410     in gconv_db.c
> (gdb) p *(&(steps[0]))
> $1 = {__shlib_handle = 0x0, __modname = 0x0, __counter = 1782250,
> __from_name = 0x7f9392c18c50 "ISO-10646/UTF8/", __to_name =
> 0x7f9394a41431 "INTERNAL",
>   __fct = 0x7f93948eace0 <__gconv_transform_utf8_internal>,
> __btowc_fct = 0x7f93948e84b0 <__gconv_btwoc_ascii>, __init_fct = 0x0,
> __end_fct = 0x0, __min_needed_from = 1,
>   __max_needed_from = 6, __min_needed_to = 4, __max_needed_to = 4,
> __stateful = 0, __data = 0x0}
> (gdb) q
>
>
> After some time when I stopped readdirs, I saw counter back to very low count:
> (gdb) b increment_counter
> Breakpoint 1 at 0x7f93948e62be
> (gdb) c
> Continuing.
> [Switching to Thread 0x7f938e0e6700 (LWP 22822)]
>
> Breakpoint 1, find_derivation (toset=toset@entry=0x7f938e0e3160
> "UTF-8//", toset_expand=0x7f9392c17ae8 "ISO-10646/UTF8/",
> fromset=fromset@entry=0x7f938e0e3140 "WCHAR_T//",
>     fromset_expand=fromset_expand@entry=0x7f9392c17a2a "INTERNAL",
> handle=handle@entry=0x7f938e0e30d0,
> nsteps=nsteps@entry=0x7f938e0e30e0) at gconv_db.c:426
> 426     gconv_db.c: No such file or directory.
> (gdb) s
> 0x00007f93948e62be in increment_counter (nsteps=1, steps=0x6a8d90) at
> gconv_db.c:410
> 410     in gconv_db.c
> (gdb) p *(&(steps[0]))
> $1 = {__shlib_handle = 0x0, __modname = 0x0, __counter = 26,
> __from_name = 0x6a8e00 "INTERNAL", __to_name = 0x6a8e20
> "ISO-10646/UTF8/",
>   __fct = 0x7f93948ea200 <__gconv_transform_internal_utf8>,
> __btowc_fct = 0x0, __init_fct = 0x0, __end_fct = 0x0,
> __min_needed_from = 4, __max_needed_from = 4, __min_needed_to = 1,
>   __max_needed_to = 6, __stateful = 0, __data = 0x0}
>
> I have few questions after looking at this behaviour:
> 1. Is there a chance of __counter getting integer overflow and getting
> reset to 0 (after 2^32)?
> 2. When do we see steps getting freed? Is it via free_derivation?
>
> Thanks
> Abhidnya
>
> On Thu, Nov 14, 2019 at 11:04 PM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Abhidnya Joshi:
> >
> > > Is there a possibility of integer overflow? I can see that counter
> > > keeps increasing even after icon_open, iconv and iconv_close is used
> > > in a sequence but multiple times for encoding.
> >
> > There could be.  The reference counter handling is strange.  I haven't
> > seen this particular bug, but I wouldn't be surprised if it existed.
> >
> > Thanks,
> > Florian
> >