Re: [PATCH v2] msvcrt: Introduce vsprintf & vswprintf helper functions.

[Piotr Caban]

Hi Gijs,

It might be better to add some tests and fix error conditions first. It 
will be easier to see how to implement the helper/if one helper is 
suitable in this case). Some quick testing shows that there are 3 
different behaviors when buffer is too small (sprintf, sprintf_p and 
sprintf_s).

On 01/14/18 03:45, Gijs Vermeulen wrote:
> Signed-off-by: Gijs Vermeulen
>
> dlls/msvcrt/wcs.c | 318 +++++++++++++++++++++++-------------------------------
>   1 file changed, 132 insertions(+), 186 deletions(-)
>
> diff --git a/dlls/msvcrt/wcs.c b/dlls/msvcrt/wcs.c
> index 098e0d5ba8..fb1173c859 100644
> --- a/dlls/msvcrt/wcs.c
> +++ b/dlls/msvcrt/wcs.c
> @@ -720,20 +720,117 @@ printf_arg arg_clbk_positional(void *ctx, int pos, int type, __ms_va_list *valis
>       return args[pos];
>   }
>
> +static int vsprintf_helper(DWORD options, char *str, MSVCRT_size_t sizeOfBuffer, MSVCRT_size_t count,
> -
>    const char *format, MSVCRT__locale_t locale, __ms_va_list valist)
>
> +{
> - static const char nullbyte = '\0';
> - printf_arg args_ctx[MSVCRT__ARGMAX+1];
> - struct _str_ctx_a puts_ctx;
> - int len, ret;
> - BOOL postional, secure, initially_positional;
> -
> - len = sizeOfBuffer;
> - postional = options & MSVCRT_PRINTF_POSITIONAL_PARAMS;
> - initially_positional = postional;
> - secure = options & MSVCRT_PRINTF_INVOKE_INVALID_PARAM_HANDLER;
> -
> - if(secure && !postional) > +        if(sizeOfBuffer>count+1 && count!=-1) len = count+1;

Please use MSVCRT__TRUNCATE instead of -1 here. The if condition can be 
also simplified if _sprintf_p behavior is fixed when too small buffer is 
passed.

You can see the incorrect behavior with following test:
errno = 0xdeadbeef;
memset(buf, 'a', sizeof(buf));
ret = _vsprintf_p_wrapper(buf, 2, "%d", 1234);
ok(ret == -1, "ret =  %d\n", ret);
ok(errno == ERANGE, "errno = %d\n", errno);
ok(!memcmp(buf, "1", 2), "buf = %s\n", buf);
> - puts_ctx.len = len;
> - puts_ctx.buf = str;
> -
> - if(postional) {
> -
>    memset(args_ctx, 0, sizeof(args_ctx));
> -
> -
>    ret = create_positional_ctx_a(args_ctx, format, valist);
> -
>    if(ret < 0) {
> -
>        MSVCRT__invalid_parameter(NULL, NULL, NULL, 0, 0);
> -
>        *MSVCRT__errno() = MSVCRT_EINVAL;
> -
>        return ret;
> -
>    } else if(!ret)
> -
>        postional = FALSE;
> - }
> -
> - ret = pf_printf_a(puts_clbk_str_a,
> -
>        &puts_ctx, format, locale, options,
> -
>        postional ? arg_clbk_positional : arg_clbk_valist,
> -
>        postional ? args_ctx : NULL,
> -
>        postional ? NULL : &valist);
> - puts_clbk_str_a(&puts_ctx, 1, &nullbyte);
> -
> - if((secure && !initially_positional) && (ret<0 || ret==len)) {
> -
>    if(count!=MSVCRT__TRUNCATE && count>sizeOfBuffer) {

You should probably check if count>=sizeOfBuffer.

Thanks,
Piotr