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");
         }
       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
Received on 2013-05-14 18:05:57 CEST