[musl] Re: [PATCH] iconv: Add check null-check for scd pointer

[musl] Re: [PATCH] iconv: Add check null-check for scd pointer

[Rich Felker]

On Sun, Mar 24, 2024 at 10:25:03PM +0300, Maks Mishin wrote:
> After having been assigned to a NULL value at iconv.c:230,
> pointer 'scd' is dereferenced at iconv.c:383.
> 
> Found by RASU JSC.
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> ---
>  src/locale/iconv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/locale/iconv.c b/src/locale/iconv.c
> index 7fb2e1ef..e0d200b8 100644
> --- a/src/locale/iconv.c
> +++ b/src/locale/iconv.c
> @@ -232,6 +232,8 @@ size_t iconv(iconv_t cd, char **restrict in, size_t *restrict inb, char **restri
>  		scd = (void *)cd;
>  		cd = scd->base_cd;
>  	}
> +	if (scd == NULL) return x;
> +
>  	unsigned to = extract_to(cd);
>  	unsigned from = extract_from(cd);
>  	const unsigned char *map = charmaps+from+1;
> -- 
> 2.30.2

This makes iconv non-functional for non-stateful conversions. The
claim by the static analysis tool is false. It is not dereferenced in
the code path where it's null because in that code path,
type==ISO2022_JP is never true.

This tool you are using is really junk. You should stop sending
untested and obviously incorrect patches to projects, and advise any
projects that have accepted your patches that they may have been
dangerously incorrect.

Rich

Re: [musl] Re: [PATCH] iconv: Add check null-check for scd pointer

[alice]

On Sun Mar 24, 2024 at 7:33 PM UTC, Rich Felker wrote:
> On Sun, Mar 24, 2024 at 10:25:03PM +0300, Maks Mishin wrote:
> > After having been assigned to a NULL value at iconv.c:230,
> > pointer 'scd' is dereferenced at iconv.c:383.
> > 
> > Found by RASU JSC.
> > 
> > Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> > ---
> >  src/locale/iconv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/locale/iconv.c b/src/locale/iconv.c
> > index 7fb2e1ef..e0d200b8 100644
> > --- a/src/locale/iconv.c
> > +++ b/src/locale/iconv.c
> > @@ -232,6 +232,8 @@ size_t iconv(iconv_t cd, char **restrict in, size_t *restrict inb, char **restri
> >  		scd = (void *)cd;
> >  		cd = scd->base_cd;
> >  	}
> > +	if (scd == NULL) return x;
> > +
> >  	unsigned to = extract_to(cd);
> >  	unsigned from = extract_from(cd);
> >  	const unsigned char *map = charmaps+from+1;
> > -- 
> > 2.30.2
>
> This makes iconv non-functional for non-stateful conversions. The
> claim by the static analysis tool is false. It is not dereferenced in
> the code path where it's null because in that code path,
> type==ISO2022_JP is never true.
>
> This tool you are using is really junk. You should stop sending
> untested and obviously incorrect patches to projects, and advise any
> projects that have accepted your patches that they may have been
> dangerously incorrect.

I'm pretty sure RASU JSC is not a tool but rather the Rusatom State Atomic
Corporation JSC, i.e. a branch at the Russian atomic energy company.

>
> Rich

Re: [musl] Re: [PATCH] iconv: Add check null-check for scd pointer

[Maks Mishin]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

Alice, that's right.

Rich, I'm sorry, but it's now always possible to test a particular function.
Can you tell me how you are testing the library?
This will help me make more meaningful patches.

пн, 25 мар. 2024 г. в 11:53, alice <alice@ayaya.dev>:

> On Sun Mar 24, 2024 at 7:33 PM UTC, Rich Felker wrote:
> > On Sun, Mar 24, 2024 at 10:25:03PM +0300, Maks Mishin wrote:
> > > After having been assigned to a NULL value at iconv.c:230,
> > > pointer 'scd' is dereferenced at iconv.c:383.
> > >
> > > Found by RASU JSC.
> > >
> > > Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> > > ---
> > >  src/locale/iconv.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/src/locale/iconv.c b/src/locale/iconv.c
> > > index 7fb2e1ef..e0d200b8 100644
> > > --- a/src/locale/iconv.c
> > > +++ b/src/locale/iconv.c
> > > @@ -232,6 +232,8 @@ size_t iconv(iconv_t cd, char **restrict in,
> size_t *restrict inb, char **restri
> > >             scd = (void *)cd;
> > >             cd = scd->base_cd;
> > >     }
> > > +   if (scd == NULL) return x;
> > > +
> > >     unsigned to = extract_to(cd);
> > >     unsigned from = extract_from(cd);
> > >     const unsigned char *map = charmaps+from+1;
> > > --
> > > 2.30.2
> >
> > This makes iconv non-functional for non-stateful conversions. The
> > claim by the static analysis tool is false. It is not dereferenced in
> > the code path where it's null because in that code path,
> > type==ISO2022_JP is never true.
> >
> > This tool you are using is really junk. You should stop sending
> > untested and obviously incorrect patches to projects, and advise any
> > projects that have accepted your patches that they may have been
> > dangerously incorrect.
>
> I'm pretty sure RASU JSC is not a tool but rather the Rusatom State Atomic
> Corporation JSC, i.e. a branch at the Russian atomic energy company.
>
> >
> > Rich
>
>

-- 
С уважением,
Максим Мишин
+7 (915) 958-41-07
maks.mishinFZ@gmail.com

[-- Attachment #2: Type: text/html, Size: 3135 bytes --]

Re: [musl] Re: [PATCH] iconv: Add check null-check for scd pointer

[Markus Wichmann]

Am Thu, Apr 11, 2024 at 08:56:56PM +0300 schrieb Maks Mishin:
> Rich, I'm sorry, but it's now always possible to test a particular function.
> Can you tell me how you are testing the library?
> This will help me make more meaningful patches.

You write tests and link them against musl, and then execute them. The
most comprehensive test suite is probably libc-test
(http://nsz.repo.hu/git/?p=libc-test), but for this it would have been
overkill. In case of something like iconv(), I would probably create a
utility similar to iconv(1), link it against musl, then write a shell
script with a few test cases. If you get the expected output, everything
is fine. If not, you broke something.

It would be harder to test something like if_nameindex(), which has a
different right answer on every given machine. But iconv() does not
depend on anything in the system. You just give it input charset, output
charset, and an input, and it generates the same output every time, or
else it is broken.

Ciao,
Markus