[musl] [PATCH] src: locale: fix potential NULL dereference in iconv() in

[musl] [PATCH] src: locale: fix potential NULL dereference in iconv() in

[Anton Moryakov]

Fixed a potential NULL dereference in iconv() by adding a check before 
accessing scd->state. If scd remains NULL due to cd & 1 != 0, 
dereferencing scd->state would cause undefined behavior.

Previous code:
    switch (scd->state) { // Potential NULL dereference

Fixed code:
    if (scd == NULL)
        goto ilseq;
    switch (scd->state) {

This ensures that scd is properly validated before usage, preventing crashes.
Although the situation where scd == NULL is unlikely, I would recommend adding this check

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov <ant.v.moryakov@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 52178950..ea3e8be1 100644
--- a/src/locale/iconv.c
+++ b/src/locale/iconv.c
@@ -380,6 +380,8 @@ size_t iconv(iconv_t cd, char **restrict in, size_t *restrict inb, char **restri
 				}
 				goto ilseq;
 			}
+			if(scd == NULL)
+				goto ilseq;
 			switch (scd->state) {
 			case 1:
 				if (c=='\\') c = 0xa5;
-- 
2.30.2

Re: [musl] [PATCH] src: locale: fix potential NULL dereference in iconv() in

[Yao Zi]

On Mon, Feb 17, 2025 at 06:18:18PM +0300, Anton Moryakov wrote:
> Fixed a potential NULL dereference in iconv() by adding a check before 
> accessing scd->state. If scd remains NULL due to cd & 1 != 0, 
> dereferencing scd->state would cause undefined behavior.
> 
> Previous code:
>     switch (scd->state) { // Potential NULL dereference
> 
> Fixed code:
>     if (scd == NULL)
>         goto ilseq;
>     switch (scd->state) {
> 
> This ensures that scd is properly validated before usage, preventing crashes.
> Although the situation where scd == NULL is unlikely, I would recommend adding this check

Why is the check necessary? I cannot find out a valid use case that the
scd pointer is NULL when converting from ISO2022_JP.

Please have a look at iconv_open().

Thanks,
Yao Zi

> Triggers found by static analyzer Svace.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@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 52178950..ea3e8be1 100644
> --- a/src/locale/iconv.c
> +++ b/src/locale/iconv.c
> @@ -380,6 +380,8 @@ size_t iconv(iconv_t cd, char **restrict in, size_t *restrict inb, char **restri
>  				}
>  				goto ilseq;
>  			}
> +			if(scd == NULL)
> +				goto ilseq;
>  			switch (scd->state) {
>  			case 1:
>  				if (c=='\\') c = 0xa5;
> -- 
> 2.30.2
> 

Re: [musl] [PATCH] src: locale: fix potential NULL dereference in iconv() in

[Anton Moryakov]

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

Thanks for the feedback!

пн, 17 февр. 2025 г. в 18:38, Yao Zi <ziyao@disroot.org>:

> On Mon, Feb 17, 2025 at 06:18:18PM +0300, Anton Moryakov wrote:
> > Fixed a potential NULL dereference in iconv() by adding a check before
> > accessing scd->state. If scd remains NULL due to cd & 1 != 0,
> > dereferencing scd->state would cause undefined behavior.
> >
> > Previous code:
> >     switch (scd->state) { // Potential NULL dereference
> >
> > Fixed code:
> >     if (scd == NULL)
> >         goto ilseq;
> >     switch (scd->state) {
> >
> > This ensures that scd is properly validated before usage, preventing
> crashes.
> > Although the situation where scd == NULL is unlikely, I would recommend
> adding this check
>
> Why is the check necessary? I cannot find out a valid use case that the
> scd pointer is NULL when converting from ISO2022_JP.
>
> Please have a look at iconv_open().
>
> Thanks,
> Yao Zi
>
> > Triggers found by static analyzer Svace.
> >
> > Signed-off-by: Anton Moryakov <ant.v.moryakov@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 52178950..ea3e8be1 100644
> > --- a/src/locale/iconv.c
> > +++ b/src/locale/iconv.c
> > @@ -380,6 +380,8 @@ size_t iconv(iconv_t cd, char **restrict in, size_t
> *restrict inb, char **restri
> >                               }
> >                               goto ilseq;
> >                       }
> > +                     if(scd == NULL)
> > +                             goto ilseq;
> >                       switch (scd->state) {
> >                       case 1:
> >                               if (c=='\\') c = 0xa5;
> > --
> > 2.30.2
> >
>

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

Re: [musl] [PATCH] src: locale: fix potential NULL dereference in iconv() in

[Thorsten Glaser]

This…

>Thanks for the feedback!

… and no response to…

>> Why is the check necessary? I cannot find out a valid use case that the
>> scd pointer is NULL when converting from ISO2022_JP.
>>
>> Please have a look at iconv_open().

… as a general pattern for these mails makes me wonder whether
someone isn’t using the mailing list as a way to train their
static analyser tool…

bye,
//mirabilos
-- 
  "Using Lynx is like wearing a really good pair of shades: cuts out
   the glare and harmful UV (ultra-vanity), and you feel so-o-o COOL."
                                         -- Henry Nelson, March 1999

Re: [musl] [PATCH] src: locale: fix potential NULL dereference in iconv() in

[Rich Felker]

On Mon, Feb 17, 2025 at 06:50:48PM +0300, Anton Moryakov wrote:
> Thanks for the feedback!
> 
> пн, 17 февр. 2025 г. в 18:38, Yao Zi <ziyao@disroot.org>:
> 
> > On Mon, Feb 17, 2025 at 06:18:18PM +0300, Anton Moryakov wrote:
> > > Fixed a potential NULL dereference in iconv() by adding a check before
> > > accessing scd->state. If scd remains NULL due to cd & 1 != 0,
> > > dereferencing scd->state would cause undefined behavior.
> > >
> > > Previous code:
> > >     switch (scd->state) { // Potential NULL dereference
> > >
> > > Fixed code:
> > >     if (scd == NULL)
> > >         goto ilseq;
> > >     switch (scd->state) {
> > >
> > > This ensures that scd is properly validated before usage, preventing
> > crashes.
> > > Although the situation where scd == NULL is unlikely, I would recommend
> > adding this check
> >
> > Why is the check necessary? I cannot find out a valid use case that the
> > scd pointer is NULL when converting from ISO2022_JP.
> >
> > Please have a look at iconv_open().

Indeed, ISO2022_JP is the primary motivation for the existence of scd
and necessarily has scd!=0. If this invariant somehow weren't met due
to a bug, faulting on the access is the desired outcome. Treating a
violation of the invariant as an illegal input sequence error is
completely the wrong thing to do.

> > > Triggers found by static analyzer Svace.

From the results so far, it sounds like this is a very poor tool.

Is it suggesting the changes and change descriptions you're sending?
If not, is there another tool layered on top of it you're using that's
generating them according to some LLM, or are you writing them
yourself?

Rich