[Spice-devel] [vdagent-win PATCH] file-xfer: handle_start: use snprintf instead of sprintf_s

[Frediano Ziglio]

> On 2/28/19 3:12 PM, Frediano Ziglio wrote:
> >> On 2/25/19 4:12 PM, Frediano Ziglio wrote:
> >>>>
> >>>> When building with older mingw, sprintf_s does not
> >>>> always work as expected, but snprintf does.
> >>>>
> >>>> Also it's more consistent in the file.
> >>>>
> >>>> Note that when building with VS, snprintf becomes sprintf_s
> >>>>
> >>>
> >>> I think this could be a bug, from documentation:
> >>>
> >>> "If the buffer is too small for the formatted text, including the
> >>> terminating null, then the buffer is set to an empty string by placing a
> >>> null character at buffer[0], and the invalid parameter handler is
> >>> invoked"
> >>>
> >>> which is different from snprintf behaviour, _snprintf is probably what do
> >>> we want.
> >>
> >> Actually, I think we do want snprintf's behavior, not _snprintf.
> >> _snprintf does not guarantee null termination of the string.
> >>
> > 
> > Yes, you are right, sorry for the confusion.
> > 
> >> We should probably check the return value.
> > 
> > Well, yes, and in call to snprintf, even on Linux. Or continue to
> > ignore as we do.
> > 
> >> Also we can add a check that there is enough space and fail early.
> >>
> > 
> > This does not make sense, better to check later, it's easier.
> > But usually you just call snprintf and ignore if truncated,
> > I don't see why in this case we should make an exception.
> 
> 
> There is a check already with +3.

That check is protecting file_path, not dest_filename.

> We can make it with +1+6, so we know there is enough space:
> 

It's not enough, but the overflow on file_path is already
checked checking the return value of MultiByteToWideChar

> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> index c456bbe..7249b21 100644
> --- a/vdagent/file_xfer.cpp
> +++ b/vdagent/file_xfer.cpp
> @@ -93,8 +93,9 @@ void
> FileXfer::handle_start(VDAgentFileXferStartMessage* start,
> 
>       wlen = _tcslen(file_path);
>       // make sure we have enough space
> -    // (1 char for separator, 1 char for filename and 1 char for NUL
> terminator)
> -    if (wlen + 3 >= MAX_PATH) {
> +    // 1 = char for directory separator: "\"
> +    const size_t POSTFIX_LEN = 6; // up to 2 digits in parentheses and
> final NUL: " (xx)"
> +    if (wlen + 1 + POSTFIX_LEN >= MAX_PATH) {
>           vd_printf("error: file too long %ls\\%s", file_path, file_name);
>           return;
>       }
> 
> Uri.
> 

Looking at the code

        char dest_filename[SPICE_N_ELEMENTS(file_name) + POSTFIX_LEN];
        if (attempt == 0) {
            strcpy(dest_filename, file_name);
        } else {
            sprintf_s(dest_filename, SPICE_N_ELEMENTS(dest_filename),
                      "%.*s (%d)%s", int(extension - file_name), file_name, attempt, extension);
        }

so the dest_filename is large enough, you could even replace sprintf_s with a sprintf.

Frediano