[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

RE: svn commit: r1482282 - svn_nls_init on Windows

From: Bert Huijben <bert_at_qqmail.nl>
Date: Tue, 14 May 2013 18:10:34 +0200

> -----Original Message-----
> From: Julian Foad [mailto:julianfoad_at_btopenworld.com]
> Sent: dinsdag 14 mei 2013 18:05
> To: BertHuijben
> Cc: dev_at_subversion.apache.org
> Subject: Re: svn commit: r1482282 - svn_nls_init on Windows
>
> Bert Huijben wrote:
> > URL: http://svn.apache.org/r1482282
> > Log:
> > * subversion/libsvn_subr/nls.c
> >   (svn_nls_init): Don't use an uninitialized variable to produce a very
> >     unlikely if not impossible to reach error.
> >
> > Modified: subversion/trunk/subversion/libsvn_subr/nls.c
> >
> ==========================================================
> ====================
> [...]
> > @@ -99,10 +98,10 @@ svn_nls_init(void)
> >
> >        if (outbytes == 0)
> >          {
> > -          err = svn_error_createf(apr_err, NULL,
> > -                                  _("Can't convert module path "
> > -                                    "to UTF-8 from UCS-2: '%s'"),
> > -                                  ucs2_path);
> > +          err = svn_error_wrap_apr(apr_get_os_error(),
> > +                                   _("Can't convert module path "
> > +                                     "to UTF-8 from UCS-2: '%s'"),
> > +                                   ucs2_path);
> >          }
>
> Hi Bert.  While you're here, I see two more problems with this piece of
code,
> just by inspection.  (I haven't tested this.)
>
> (1) Inserting a UCS-2 string into a UTF-8 error message will surely fail
in one
> way or another.
>
> (2) Just below this error handling, the code to write the null terminator
is
> bogus.
>
> I suggest:
>
> [[[
>    inwords = GetModuleFileNameW(0, ucs2_path,
>                                 sizeof(ucs2_path) / sizeof(ucs2_path[0]));
>    [...]
>      {
>
>        outbytes = outlength = 3 * (inwords + 1);
>        utf8_path = apr_palloc(pool, outlength);
>
>        outbytes = WideCharToMultiByte(CP_UTF8, 0, ucs2_path, inwords,
>                                        utf8_path, outbytes, NULL, NULL);
>
>        if (outbytes == 0)
>          {
> -          err = svn_error_createf(apr_err, NULL,
> -                                  _("Can't convert module path "
> -                                    "to UTF-8 from UCS-2: '%s'"),
> -                                  ucs2_path);
> +          err = svn_error_create(apr_err, NULL,
> +                                 _("Can't convert module path "
> +                                   "to UTF-8 from UCS-2");

apr_err is no longer defined, so this code wouldn't compile.

Most likely the second by of the UCS-2 string is 0 and in all cases the
final word is 0, so this won't crash. So we can fix this in a later release
if necessary.

>          }
>        else
>          {
> -          utf8_path[outlength - outbytes] = '\0';
> +          utf8_path[outbytes] = '\0';
>            internal_path = svn_dirent_internal_style(utf8_path, pool);

I'm surprised that this code worked correctly in 1.7... but it did. This
code path has been active in at least the SlikSvn build.

        Bert
>
> [...]
>
> ]]]
>
> The existing code puts the null terminator in the wrong place.  This will
> truncate any path that is longer than PATH_MAX/2, and will fail to
terminate
> any path shorter than PATH_MAX/2.  WideCharToMultiByte() explicitly does
> not promise to write a null terminator.  This code will work despite this
bug
> iff:
>
>   * path length < MAX_PATH/2
>
> and either of:
>
>   * WideCharToMultiByte() adds a null terminator
>   * apr_alloc() clears the memory
>
> Otherwise, it will produce the wrong result.
>
> - Julian
Received on 2013-05-14 18:11:35 CEST

This is an archived mail posted to the Subversion Dev mailing list.