Re: [PATCH] gconv: Do not emit spurious NUL character in ISO-2022-JP-3 (bug 28524)

[Florian Weimer <fweimer@redhat.com>]

* Никита Попов via Libc-alpha:

> To eliminate this issue, these steps are taken:
> * Restore original condition
> '(data->__statep->__count & ~7) != ASCII_set'.
> It is necessary since bits 0-2 may contain
> number of buffered input characters.

Right, I missed that use of __count by the framework. 8-/

> * Check that queued character is not NUL.
> Similar step is taken for main conversion loop.

And since EMIT_SHIFT_TO_INIT runs before the loop, it's necessary to
mirror that logic there.

> diff --git a/iconvdata/bug-iconv15.c b/iconvdata/bug-iconv15.c
> new file mode 100644
> index 0000000000..4037e131ff

> +  /* First call to iconv should alter internal state.
> +     Now, JISX0201_Kana_set is selected and
> +     state value != ASCII_set */

Sorry, GNU style says that comments should end with ”.  */” (period and
two spaces).

> +  TEST_VERIFY (iconv (cd, &inbuf, &inleft, &outbuf, &outleft) != (size_t) -1);

Can we add an additional test here?  I think no bytes have been added to
the output at this point, right?

> +  /* Second call shall emit spurious NUL character in unpatched glibc. */

> +  /* No characters are expected to be produced. */

Missing space after ”.” (see above).

> diff --git a/iconvdata/iso-2022-jp-3.c b/iconvdata/iso-2022-jp-3.c
> index 70b28ace7f..5e66d686f1 100644
> --- a/iconvdata/iso-2022-jp-3.c
> +++ b/iconvdata/iso-2022-jp-3.c
> @@ -79,20 +79,31 @@ enum
>     the output state to the initial state.  This has to be done during the
>     flushing.  */
>  #define EMIT_SHIFT_TO_INIT \
> -  if (data->__statep->__count != ASCII_set)			      \
> +  if ((data->__statep->__count & ~7) != ASCII_set)			      \
>      {									      \
>        if (FROM_DIRECTION)						      \
>  	{								      \
> -	  if (__glibc_likely (outbuf + 4 <= outend))			      \
> +	  uint32_t ch = data->__statep->__count >> 6;			      \
> +									      \
> +	  if (__glibc_unlikely (ch != 0))				      \
>  	    {								      \
> -	      /* Write out the last character.  */			      \
> -	      *((uint32_t *) outbuf) = data->__statep->__count >> 6;	      \
> -	      outbuf += sizeof (uint32_t);				      \
> -	      data->__statep->__count = ASCII_set;			\
> +	      if (__glibc_likely (outbuf + 4 <= outend))		      \
> +		{							      \
> +		  /* Write out the last character.  */			      \
> +		  put32u (outbuf, ch);					      \
> +		  outbuf += 4;						      \
> +		  data->__statep->__count &= 7;				      \
> +		  data->__statep->__count |= ASCII_set;			      \
> +		}							      \
> +	      else							      \
> +		/* We don't have enough room in the output buffer.  */	      \
> +		status = __GCONV_FULL_OUTPUT;				      \
>  	    }								      \
>  	  else								      \
> -	    /* We don't have enough room in the output buffer.  */	      \
> -	    status = __GCONV_FULL_OUTPUT;				      \
> +	    {								      \
> +	      data->__statep->__count &= 7;				      \
> +	      data->__statep->__count |= ASCII_set;			      \
> +	    }								      \
>  	}								      \
>        else								      \
>  	{								      \

The actual code change looks okay to me.

Thanks,
Florian