> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel