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

[Andrew Savchenko]

Hi,

On Tuesday 29 April 2008 21:49, Reimar Döffinger wrote:
[...]
> > > 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.

Ok, I'll remove it.

> > > 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.

Yes, I missed that; static qualifiers will be kept.

> > --- 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.

Done.

> > +#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.

I'll make comment in mp_msg.h doxy-compatible.

> > +    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

Surely. This code was designed with the expectation of 
mp_recode_to() will never be called with NULL, or improperly 
allocated buffer, or zero buf_size. This was done in order to 
avoid extra checks, and in current code, where buffer is always 
allocated prior mp_recode_to() call, this should work fine. 

However, if mp_recode_to() fault-tolerance is required, I'll do 
this. Performance shouldn't be an issue for this function.

> > +            // 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);

Well, I'm thinking about offset calculation in another way... I can 
use memcpy of course, but this seems ugly for me. 
I don't want to recall iconv() from scratch due to performance 
issues: this function may be very slow.

> It's probably a good idea to do this for all errors.

At least for EILSEQ error code this will be a problem. EILSEQ 
occurs when invalid multibyte sequence was encountered, so buffer 
will be grown until out of memory. Perhaps we should just copy the 
problem character as is and issue a warning? This is still better 
than nothing.

> > @@ -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.

Ok. Due to technical reasons I may be off-line during next two or 
three weeks, so pardon for this delay.

With best regards,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080502/08f903ee/attachment.pgp>