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

[Thomas Guillem]

On Fri, Jun 23, 2017, at 12:10, 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 );
OK but this is still not sane, you should add a goto error just after
the errors logs. It's better to return nothing here than something that
is badly or not converted.
> -- 
> 2.13.1
> 
> On Thu, Jun 22, 2017 at 5:45 PM, Rémi Denis-Courmont
> <remi at remlab.net> wrote:> 
>> 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.> 
> Also "goto error" when iconv_open fails is going to result in a
> invalid free of p_line.
free(NULL) is valid, it's a NO-OP, we do it a lot in the VLC codebase.

> 
> Besides if you are not going to provide any constructive review and
> take someone else's patches and try to implement it yourself, without
> posting on the mailing list and be counter-productive then you might
> just as well do a fork.
This is not an acceptable way of talking to one of the biggest
contributors of the VLC project (and the same for every others members).
>> 
>> --
>>  Rémi Denis-Courmont
>>  Typed on an inconvenient virtual keyboard
> 
> _________________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170623/35fbea2c/attachment.html>