>
> >
> > 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
With the lto options I get (xxx is a version of FileXfer::handle_start
stripped of FileXfer stuff):
4017c8: e8 00 00 00 00 callq 4017cd <_Z3xxxPKc+0x1fd>
while without I get
40934b: e8 00 ba 12 00 callq 534d50 <_Z7sprintfPcPKcz>
(the sprintf call)
Weirdly I cannot find any relocation for 4017c9. Not that I'm actually
expecting any. I would expect an indirect call for an imported function
or a relative call with no relocation (maybe to the import function).
So... it is a bug on the compiler or binutils? I can note a bug in
objdump not showing correctly relocations. I used even dumpbin , this
has not the relocation bug as objdump but it confirms that there's
no relocation on 4017c9.
Some more digging. Looks like using -save-temps, strace and removing
-pipe options g++ calls "as" with a proper .s file with the correct call
(sprintf) and "as" correctly produce an .o file with reference to
sprintf but after this ld take this .o file and produce a wrong
executable. So looks like is either ld of the lto plugin (passed to
ld).
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel