[vlc-devel] [PATCH] stream: iconv: convert only if we have a conversion descriptor

[Rémi Denis-Courmont]

On vendredi 23 juin 2017 15:10:51 EEST Shaleen Jain wrote:
>  From d274065aeb20b6538a108bb310e7c8ab2445e5fd Mon Sep 17 00:00:00 2001
>  From: Shaleen Jain <shaleen.jain95 at gmail.com>
> Date: Fri, 23 Jun 2017 15:26:40 +0530
> Subject: [PATCH] stream: iconv: convert only if we have a conversion
> descriptor
> 
> ---
>  src/input/stream.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/input/stream.c b/src/input/stream.c
> index affc7b2c0d..ecdaa2f997 100644
> --- a/src/input/stream.c
> +++ b/src/input/stream.c
> @@ -195,6 +195,7 @@ char *vlc_stream_ReadLine( stream_t *s )
>          if( i_pos == 0 && i_data >= 2 )
>          {
>              const char *psz_encoding = NULL;
> +          priv->text.conv = (vlc_iconv_t)-1;
> 
>              if( unlikely(priv->text.conv != (vlc_iconv_t)-1) )
>              {   /* seek back to beginning? reset */
> @@ -220,7 +221,7 @@ char *vlc_stream_ReadLine( stream_t *s )
>                  if( unlikely(priv->text.conv == (vlc_iconv_t)-1) )
>                  {
>                      msg_Err( s, "iconv_open failed" );
> -                    goto error;
> +            	     return NULL;
>                  }
>                  priv->text.char_width = 2;
>              }
> @@ -327,7 +328,8 @@ char *vlc_stream_ReadLine( stream_t *s )
>              p_in = p_line;
>              p_out = psz_new_line;
> 
> -            if( vlc_iconv( priv->text.conv, &p_in, &i_in, &p_out,
> &i_out ) == (size_t)-1 )
> +          if( priv->text.conv != (vlc_iconv_t)-1 &&
> +            	        vlc_iconv( priv->text.conv, &p_in, &i_in, &p_out,
> &i_out ) == (size_t)-1 )
>              {
>                  msg_Err( s, "conversion error: %s", vlc_strerror_c(
> errno ) );
>                  msg_Dbg( s, "original: %d, in %zu, out %zu", i_line,
> i_in, i_out );
> 
> > Le 22 juin 2017 14:44:49 GMT+03:00, Shaleen Jain
> > 
> > <shaleen.jain95 at gmail.com> a écrit :
> >> On Wed, Jun 21, 2017 at 11:38 PM, Rémi Denis-Courmont
> >> 
> >> <remi at remlab.net> wrote:
> >>> Le keskiviikkona 21. kesäkuuta 2017, 22.55.37 EEST Shaleen Jain a
> >>> 
> >>> écrit :
> >>>>  How so?
> >>> 
> >>> It´s still not resulting in anything sane.
> >> 
> >> I'm sorry I don't see how the logic is incorrect,
> >> if priv->text.conv is -1 (which will happen if vlc_iconv_open()
> >> fails)
> >> priv->text.conv != (vlc_iconv_t)(-1) would be false and vlc_iconv()
> >> would not run, otherwise it would proceed to do the conversion.
> >> 
> >>> --
> >>> 雷米‧德尼-库尔蒙
> >>> https://www.remlab.net/
> >>> _______________________________________________
> >>> vlc-devel mailing list
> >>> To unsubscribe or modify your subscription options:
> >>> https://mailman.videolan.org/listinfo/vlc-devel
> > 
> > Skipping the conversion is not going to result in sane behaviour.
> 
> It isn't suppose to do anything sane by itself, it's a sanity check for
> when psz_encoding is null and the program doesn't need to do a
> conversion.

AFAICT, that never was a possibility in the first place. We never have 
multibyte without one UTF-16 endianess or the other. There only was the 
theoretical possibility that iconv_open() failed.

> Also "goto error" when iconv_open fails is going to result in a invalid
> free of p_line.
> 
> Besides if you are not going to provide any constructive review

I've seen far less constructive reviews here and elsewhere.

> and
> take someone else's patches and try to implement it yourself,

I did not take anybody else's patches. I fixed aliasing problems as a 
consequence of the libav-devel aliasing discussion. Then I found the leak and 
initialization error and fixed those too.

The annoyance of having your patches mooted or partially mooted is not an 
excuse for lashing out.

> without
> posting on the mailing list and be counter-productive then you might
> just as well do a fork.

There are way more controversial, sensitive, or broken patches getting pushed 
without review, and it was much worse before. It is rather silly to blame me 
for this collective project practice. Plus it's not my fault that there are 
too few people, and with too few independent affiliations for systematic and 
unbiased code reviews.

-- 
Rémi