[Spice-devel] [PATCH vd_agent_win] Use sprintf_s instead of sprintf to not crash

[Frediano Ziglio]

> 
> On Tue, 2017-05-23 at 07:01 -0400, Frediano Ziglio wrote:
> > > 
> > > On Wed, May 17, 2017 at 12:57:34PM -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > > 
> > > > > > ---
> > > > > >  vdagent/file_xfer.cpp | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> > > > > > index de1aea1..de98d50 100644
> > > > > > --- a/vdagent/file_xfer.cpp
> > > > > > +++ b/vdagent/file_xfer.cpp
> > > > > > @@ -113,8 +113,8 @@ void
> > > > > > FileXfer::handle_start(VDAgentFileXferStartMessage*
> > > > > > start,
> > > > > >          if (attempt == 0) {
> > > > > >              strcpy(dest_filename, file_name);
> > > > > >          } else {
> > > > > > -            sprintf(dest_filename, "%.*s (%d)%s",
> > > > > > int(extension -
> > > > > > file_name), file_name,
> > > > > > -                    attempt, extension);
> > > > > > +            sprintf_s(dest_filename,
> > > > > > SPICE_N_ELEMENTS(file_name) +
> > > > > > POSTFIX_LEN,
> > > > > > +                      "%.*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) {
> > > > > >              vd_printf("failed converting file_name:%s to
> > > > > > WideChar",
> > > > > >              dest_filename);
> > > > > 
> > > > > As far as I know dest_filename is allocate big enough to avoid
> > > > > the
> > > > > overflow.
> > > > > Did it crash for you or just a warning from the compiler?
> > > > > 
> > > > > This would be easier
> > > > > 
> > > > >             sprintf_s(dest_filename,
> > > > > SPICE_N_ELEMENTS(dest_filename),
> > > > >                       "%.*s (%d)%s", int(extension -
> > > > > file_name),
> > > > >                       file_name,
> > > > >                       attempt, extension);
> > > > > 
> > > > > without computing again the length.
> > > > > 
> > > > 
> > > > Got some discussion with Jakub and Pavel on IRC.
> > > > They confirmed that with Fedora 26 and 27 the crash happens.
> > > > I tried to do some tests (removed the CreateFile and some calls
> > > > to have
> > > > a test function) with Fedora 25 but didn't manage to reproduce.
> > > > Jakub reported that this happens with small names.
> > > > 
> > > > Maybe is an issue with MingW with newer Fedora? Worth
> > > > investigating.
> > > 
> > > This is pushed already, but this should have been added to the
> > > commit
> > > log.
> > > 
> > > Christophe
> > > 
> > 
> > I think would be better to open a bug trying to understand where the
> > problem
> > is. The source should not crash with old code.
> > I spent last 2 hours trying to compile the agent using mock (my
> > machine
> 
> you can use our copr, it is built in all supported Fedoras there. msi
> should be in the rpm
> 
> https://copr.fedorainfracloud.org/coprs/g/spice/nightly/package/mingw-
> spice-vdagent/
> 
> > is using Fedora 25) but not managed so far.
> > I was trying to write a patch to build a test case but with Fedora
> > 25
> > and Wine the test case works correctly.
> > 
> > Frediano

Thanks.

I'm actually setup a virtual machine. So far is working. Which is the good
news.
The bad news is that removing -flto -fwhole-program options from the options
fix the issue :-( so... is a compiler bug or if these options are enabled
something inside the includes provide different (wrong) implementation?
Another good news is that wine is failing too (so no need to copy and test
on Windows).

Frediano