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