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

Submitted by Uri Lublin on Feb. 25, 2019, 10:20 a.m.

Details

Message ID 20190225102006.22980-1-uril@redhat.com
State New
Headers show
Series "file-xfer: handle_start: use snprintf instead of sprintf_s" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Uri Lublin Feb. 25, 2019, 10:20 a.m.
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

Related: rhbz#1410181

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 vdagent/file_xfer.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

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) {

Comments

> 
> 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
> 
> Related: rhbz#1410181
> 
> Signed-off-by: Uri Lublin <uril@redhat.com>

Yes, you are right.
I think the change SPICE_N_ELEMENTS -> sizeof is not needed.
In this case does not make much difference but if tomorrow
we move to wide character is better to keep SPICE_N_ELEMENTS
as we will need to pass the "character" size, not the byte size.

Frediano

> ---
>  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) {
On Mon, Feb 25, 2019 at 12:20:06PM +0200, Uri Lublin 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 really don't mind to have this patch (I might have proposed it
long time ago even, not sure) but overall this should have been
fixed by rebase on mingw-crt, no?

Cheers,

> Related: rhbz#1410181
> 
> Signed-off-by: Uri Lublin <uril@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) {
> -- 
> 2.20.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 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.

> Related: rhbz#1410181
> 
> Signed-off-by: Uri Lublin <uril@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
On 2/25/19 2:08 PM, Victor Toso wrote:
> On Mon, Feb 25, 2019 at 12:20:06PM +0200, Uri Lublin 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 really don't mind to have this patch (I might have proposed it
> long time ago even, not sure) but overall this should have been
> fixed by rebase on mingw-crt, no?

I think it should be fixed by a rebase of mingw-crt.

Do you want me to mention it in the commit log ?

Thanks,
     Uri.

> 
> Cheers,
> 
>> Related: rhbz#1410181
>>
>> Signed-off-by: Uri Lublin <uril@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) {
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
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.

We should probably check the return value.
Also we can add a check that there is enough space and fail early.

Uri.

> 
>> Related: rhbz#1410181
>>
>> Signed-off-by: Uri Lublin <uril@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
>
On 2/25/19 1:15 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
>>
>> Related: rhbz#1410181
>>
>> Signed-off-by: Uri Lublin <uril@redhat.com>
> 
> Yes, you are right.
> I think the change SPICE_N_ELEMENTS -> sizeof is not needed.
> In this case does not make much difference but if tomorrow
> we move to wide character is better to keep SPICE_N_ELEMENTS
> as we will need to pass the "character" size, not the byte size.

I can change it back, but

Both Linux and Windows documentation mention that snprintf's
second argument is in bytes (although in the Windows one
it's a bit confusing).

If we change the type we better change the snprintf
as well (either the format string or the function itself).


Uri.

> 
> Frediano
> 
>> ---
>>   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) {
Hi,

On Thu, Feb 28, 2019 at 11:04:51AM +0200, Uri Lublin wrote:
> On 2/25/19 2:08 PM, Victor Toso wrote:
> > On Mon, Feb 25, 2019 at 12:20:06PM +0200, Uri Lublin 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 really don't mind to have this patch (I might have proposed it
> > long time ago even, not sure) but overall this should have been
> > fixed by rebase on mingw-crt, no?
> 
> I think it should be fixed by a rebase of mingw-crt.
> 
> Do you want me to mention it in the commit log ?

Looking at tags that contain the fix:

$ git tag --contains 9975303
v5.0-rc1
v5.0-rc2
v5.0.0
v5.0.1
v5.0.2
v5.0.3
v5.0.4
v6.0.0

So perhaps adding `Fixed in mingw-w64 at v5.0.0 by 9975303 (CRT:
vsprintf_s calling wrong function)` would be fine.

This tag is from Oct 17, 2016 so indeed, quite old!

Cheers,

> Thanks,
>     Uri.
> 
> > 
> > Cheers,
> > 
> > > Related: rhbz#1410181
> > > 
> > > Signed-off-by: Uri Lublin <uril@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) {
> > > -- 
> > > 2.20.1
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On 2/28/19 11:38 AM, Victor Toso wrote:
> Hi,
> 
> On Thu, Feb 28, 2019 at 11:04:51AM +0200, Uri Lublin wrote:
>> On 2/25/19 2:08 PM, Victor Toso wrote:
>>> On Mon, Feb 25, 2019 at 12:20:06PM +0200, Uri Lublin 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 really don't mind to have this patch (I might have proposed it
>>> long time ago even, not sure) but overall this should have been
>>> fixed by rebase on mingw-crt, no?
>>
>> I think it should be fixed by a rebase of mingw-crt.
>>
>> Do you want me to mention it in the commit log ?
> 
> Looking at tags that contain the fix:
> 
> $ git tag --contains 9975303
> v5.0-rc1
> v5.0-rc2
> v5.0.0
> v5.0.1
> v5.0.2
> v5.0.3
> v5.0.4
> v6.0.0
> 
> So perhaps adding `Fixed in mingw-w64 at v5.0.0 by 9975303 (CRT:
> vsprintf_s calling wrong function)` would be fine.
> 
> This tag is from Oct 17, 2016 so indeed, quite old!

I'll add it.

Thanks,
     Uri.
> 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@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
> > 
> 
>