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");
}
else
{
- utf8_path[outlength - outbytes] = '\0';
+ utf8_path[outbytes] = '\0';
internal_path = svn_dirent_internal_style(utf8_path, pool);
[...]
]]]
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
