[MPlayer-dev-eng] [PATCH] ftp charset selection support

[Reimar Döffinger]

On Sun, Apr 20, 2008 at 06:11:43AM +0400, Andrew Savchenko wrote:
> On Sunday 20 April 2008 00:31, Reimar Döffinger wrote:
> > > +const char* mp_recode_to(iconv_t* const icnv,
> > > +    const char* const filename,
> > > +    const char* const tocode, const char* const fromcode,
> > > +    char** const buf, size_t* const buf_size);
> > > +#endif /* USE_ICONV */
> > > +
> > > +const char* filename_recode(const char* const filename);
> >
> > I see now point in about half of those consts, they IMO just
> > obfuscate the code, 
> 
> They are harmless, besides const qualifier protects from some 
> development errors and may help compiler to optimize code or may 
> not, but nevertheless they change nothing in the worst case. The 
> only drawback is a little code readability decreasing, so perhaps 
> they are worth to be used.

At least the second const for filename I can not really see much of a
point in.

> > > --- mplayer_base/mp_msg.c	2008-04-12 20:24:22.000000000 +0400
> > > +++ mplayer/mp_msg.c	2008-04-19 21:46:20.000000000 +0400
> > > @@ -31,38 +31,74 @@
> > >  int mp_msg_module = 0;
> > >  #ifdef USE_ICONV
> > >  char *mp_msg_charset = NULL;
> > > -static char *old_charset = NULL;
> > > -static iconv_t msgiconv;
> > > -#endif
> > > -
> > > -const char* filename_recode(const char* filename)
> > > +char *old_charset = NULL;
> > > +iconv_t msgiconv;
> > > +iconv_t inv_msgiconv = (iconv_t)(-1);
> >
> > Why removing the static? 
> 
> They are in global scope, so static by default. Static declarations 
> are absolutely useless here, so they was removed. Well, if you 
> insist I'll keep them, anyway they change nothing.

Read up on C. They are file-local scope with static and project-global
without, which is a big difference.

> --- mplayer_base/mp_msg.c	2008-04-12 20:24:22.000000000 +0400
> +++ mplayer/mp_msg.c	2008-04-20 05:53:40.000000000 +0400
> @@ -33,7 +33,9 @@
>  char *mp_msg_charset = NULL;
>  static char *old_charset = NULL;
>  static iconv_t msgiconv;
> -#endif
> +char *recoded_filename;
> +size_t recoded_filename_size = MSGSIZE_MAX;
> +#endif /* USE_ICONV */

And thus these should be static as well.

> +#ifdef USE_ICONV
> +// recode filename from <tocode> to <fromcode> charset
> +const char* mp_recode_to(iconv_t* const icnv, 

Function comments should be doxygen-compatible, though if the one in .h already
is just remove this one IMO.

> +    max_path = *buf_size - 1;
> +    precoded = *buf;
> +    while(1) {
> +        if (iconv(*icnv, &filename, &filename_len,
> +                  &precoded, &max_path) == (size_t)(-1)) {

This fails in all kinds of ways for *buf_size == 0

> +            // if buffer size is not enough, sufficient memory 
> +            // must be allocated; 
> +            if (errno == E2BIG) {
> +                unsigned int diff = precoded - *buf;
> +                // double buffer
> +                max_path += *buf_size;
> +                *buf_size <<= 1;
> +                *buf = realloc(*buf, *buf_size);
> +                precoded = (unsigned int)(*buf) + diff;

This as well is very wrong. It is just wrong on 64 bit systems, it is
possibly exploitable (since it does not check for *buf == NULL) and it
seems overcomplicated to me anyway, IMO just restart from scratch by
calling iconv(*icnv, NULL, NULL, NULL, NULL);
It's probably a good idea to do this for all errors.

> @@ -77,6 +115,9 @@
>      mp_msg_charset = getenv("MPLAYER_CHARSET");
>      if (!mp_msg_charset)
>        mp_msg_charset = get_term_charset();
> +    // allocate initial buffer for filename conversion 
> +    // via filename_recode()
> +    recoded_filename = malloc(recoded_filename_size);

IMO fix the code to work with buf = NULL and buf_size = 0 and get rid of
this.
I have not reviewed the other patch.

Greetings,
Reimar Döffinger