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

[Frediano Ziglio]

> 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.

> Uri.
> 
> > 
> >> Related: rhbz#1410181
> >>
> >> Signed-off-by: Uri Lublin <uril at redhat.com>
> >> ---
> >>   vdagent/file_xfer.cpp | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> >> index ada3b47..c456bbe 100644
> >> --- a/vdagent/file_xfer.cpp
> >> +++ b/vdagent/file_xfer.cpp
> >> @@ -113,7 +113,7 @@ void
> >> FileXfer::handle_start(VDAgentFileXferStartMessage*
> >> start,
> >>           if (attempt == 0) {
> >>               strcpy(dest_filename, file_name);
> >>           } else {
> >> -            sprintf_s(dest_filename, SPICE_N_ELEMENTS(dest_filename),
> >> +            snprintf(dest_filename, sizeof(dest_filename),
> >>                         "%.*s (%d)%s", int(extension - file_name),
> >>                         file_name,
> >>                         attempt, extension);
> >>           }
> >>           if ((MultiByteToWideChar(CP_UTF8, 0, dest_filename, -1,
> >>           file_path +
> >>           wlen, MAX_PATH - wlen)) == 0) {
> > 
> > Frediano
> > 
> 
>