[PATCH v2 1/2] Cygwin: pty: Revise convert_mb_str() function.

[Corinna Vinschen]

On Sep  9 22:40, Takashi Yano via Cygwin-patches wrote:
> - Use tmp_pathbuf instead of HeapAlloc()/HeapFree().
> - Remove mb_str_free() function.
> - Consider the case where the multibyte string stops in the middle
>   of a multibyte char.
> ---
>  winsup/cygwin/fhandler_tty.cc | 118 ++++++++++++++++++++++------------
>  1 file changed, 77 insertions(+), 41 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index 6de591d9b..c4b7fc61d 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -26,6 +26,7 @@ details. */
>  #include <asm/socket.h>
>  #include "cygwait.h"
>  #include "registry.h"
> +#include "tls_pbuf.h"
>  
>  #ifndef PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE
>  #define PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE 0x00020016
> @@ -116,40 +117,72 @@ CreateProcessW_Hooked
>    return CreateProcessW_Orig (n, c, pa, ta, inh, f, e, d, si, pi);
>  }
>  
> -static char *
> -convert_mb_str (UINT cp_to, size_t *len_to,
> -		UINT cp_from, const char *ptr_from, size_t len_from)
> +static void
> +convert_mb_str (UINT cp_to, char *ptr_to, size_t *len_to,
> +		UINT cp_from, const char *ptr_from, size_t len_from,
> +		mbstate_t *stat)

Heh, it's funny to use mbstate_t together with Windows functions :)
However, the var name `stat' makes me itch.  It looks too close to
struct stat and function stat.  What about "mbp" or something along
these lines?

>  {
> -  char *buf;
>    size_t nlen;
> +  tmp_pathbuf tp;
>    if (cp_to != cp_from)
>      {
> -      size_t wlen =
> -	MultiByteToWideChar (cp_from, 0, ptr_from, len_from, NULL, 0);
> -      wchar_t *wbuf = (wchar_t *)
> -	HeapAlloc (GetProcessHeap (), 0, wlen * sizeof (wchar_t));
> -      wlen =
> -	MultiByteToWideChar (cp_from, 0, ptr_from, len_from, wbuf, wlen);
> -      nlen = WideCharToMultiByte (cp_to, 0, wbuf, wlen, NULL, 0, NULL, NULL);
> -      buf = (char *) HeapAlloc (GetProcessHeap (), 0, nlen);
> -      nlen = WideCharToMultiByte (cp_to, 0, wbuf, wlen, buf, nlen, NULL, NULL);
> -      HeapFree (GetProcessHeap (), 0, wbuf);
> +      wchar_t *wbuf = tp.w_get ();
> +      int wlen = 0;
> +      if (cp_from == 65000) /* UTF-7 */

CP_UTF7?

> +	/* MB_ERR_INVALID_CHARS does not work properly for UTF-7.
> +	   Therefore, just convert string without checking */
> +	wlen = MultiByteToWideChar (cp_from, 0, ptr_from, len_from,
> +				    wbuf, NT_MAX_PATH);
> +      else
> +	{
> +	  char *tmpbuf = tp.c_get ();
> +	  memcpy (tmpbuf, stat->__value.__wchb, stat->__count);
> +	  if (stat->__count + len_from > NT_MAX_PATH)
> +	    len_from = NT_MAX_PATH - stat->__count;
> +	  memcpy (tmpbuf + stat->__count, ptr_from, len_from);
> +	  int total_len = stat->__count + len_from;
> +	  stat->__count = 0;
> +	  int mblen = 0;
> +	  for (const char *p = tmpbuf; p < tmpbuf + total_len; p += mblen)
> +	    /* Max bytes in multibyte char is 4. */
> +	    for (mblen = 1; mblen <= 4; mblen ++)
> +	      {
> +		/* Try conversion */
> +		int l = MultiByteToWideChar (cp_from, MB_ERR_INVALID_CHARS,
> +					     p, mblen,
> +					     wbuf + wlen, NT_MAX_PATH - wlen);
> +		if (l)
> +		  { /* Conversion Success */
> +		    wlen += l;
> +		    break;
> +		  }
> +		else if (mblen == 4)
> +		  { /* Conversion Fail */
> +		    l = MultiByteToWideChar (cp_from, 0, p, 1,
> +					     wbuf + wlen, NT_MAX_PATH - wlen);
> +		    wlen += l;
> +		    mblen = 1;
> +		    break;
> +		  }
> +		else if (p + mblen == tmpbuf + total_len)
> +		  { /* Multibyte char incomplete */
> +		    memcpy (stat->__value.__wchb, p, mblen);
> +		    stat->__count = mblen;
> +		    break;
> +		  }
> +		/* Retry conversion with extended length */
> +	      }
> +	}
> +      nlen = WideCharToMultiByte (cp_to, 0, wbuf, wlen,
> +				  ptr_to, *len_to, NULL, NULL);
>      }
>    else
>      {
>        /* Just copy */
> -      buf = (char *) HeapAlloc (GetProcessHeap (), 0, len_from);
> -      memcpy (buf, ptr_from, len_from);
> -      nlen = len_from;
> +      nlen = min (*len_to, len_from);
> +      memcpy (ptr_to, ptr_from, nlen);

if the *caller* checks for cp_to != cp_from, this memcpy could go
away and the caller could just use the already existing buffer.

Other than that, LGTM.


Thanks,
Corinna