> -----Original Message-----
> From: Julian Foad [mailto:julianf...@btopenworld.com]
> Sent: dinsdag 14 mei 2013 18:05
> To: BertHuijben
> Cc: dev@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
