[Spice-devel] [spice-gtk v1] file-xfer: Fix bad filename encoding

[Frediano Ziglio]

> 
> Hi,
> 
> On Wed, Apr 12, 2017 at 07:19:54AM -0400, Frediano Ziglio wrote:
> >
> > >
> > > From: Victor Toso <me at victortoso.com>
> > >
> > > Manual for G_FILE_ATTRIBUTE_STANDARD_NAME states:
> > >  > The name is the on-disk filename which may not be in any known
> > >  > encoding, and can thus not be generally displayed as is.
> > >
> > > Considering a file named "ěščřžýáíé", if we use
> > > G_FILE_ATTRIBUTE_STANDARD_NAME get the file name, we will have the
> > > following 72 char long string:
> > > "\xc4\x9b\xc5\xa1\xc4\x8d\xc5\x99\xc5\xbe\xc3\xbd\xc3\xa1\xc3\xad\xc3\xa9"
> > >
> >
> > this string is only 18 characters long, why 72 ?
> 
> strlen(basename) == 72, there are 72 chars in above string.
> 
> > > We should be use G_FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME instead which
> > > will give us the correct 18 long utf-8 string: "ěščřžýáíé"
> >
> > I think this solves the encoding as we'll transmit with a given encoding
> > (utf8).
> 
> Yes
> 
> > If the source filename is not correctly encoded this will give a
> > destination filename different from the source.
> 
> That was the bug
> 
> > As the protocol does not include an encoding utf-8 is a good choice
> > as ASCII compatible and in theory should be able to encode every
> > possible name set (beside invalid one of course).
> 
> Yeah, In the commit [0] and related messages I did not find a specific
> encoding that should be used, maybe it is stated somewhere else... I
> agree that utf8 is a good choice.
> 
> [0]
> https://cgit.freedesktop.org/spice/spice-protocol/commit/?id=a484ca8095556f7f75979f14
> 
> > As we deal with different OSes and encodings (ie utf8 in Linux and
> > utf16 in Windows) having a super set is probably the best solution.
> > A question could be: Is is better
> >   basename = g_file_info_get_attribute_as_string(info,
> >   G_FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME);
> > or
> >   basename = g_file_info_get_attribute_as_byte_string(info,
> >   G_FILE_ATTRIBUTE_STANDARD_NAME);
> > (as my previous consideration I think the first).
> 
> Agreed
> 
> > Maybe the name G_FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME is
> > confusing here as we just want the standard utf-8 encoded
> > name, not something just to display to the user.
> > A comment could help.
> 
> I too thought it was confusing hence the bug :)
> 
> I should have payed more attention to the documentation at the time I
> refactored this bits. [ before we were using g_file_get_basename() but I
> changed to g_file_info_get_attribute_as_string() ].
> 
> Besides the quote from the manual and the explanation of the issue, not
> sure what comment should I provide or what is lacking in explanation.
> 

Maybe is just me thinking "display name" as something related to
the name used for display purposes which could be entirely different
from the real name.
The code is attempting to get file name encoded for the network,
so that's why "display name" could be misleading. Even the documentation
state that the display name is "A display name is guaranteed to be in
UTF8 and can thus be displayed in the UI" but it does not say which
kind of transformations are performed in order to achieve this.
Does it strip spaces at the end or beginning or remove consecutive ones?
Does it strip quotes or special characters? In theory a Unix filename
could contain an end of line or other control characters.

>     toso
> >
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1440206
> > > 
> > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > ---
> > >  src/channel-main.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > index be7c852..c8de5f3 100644
> > > --- a/src/channel-main.c
> > > +++ b/src/channel-main.c
> > > @@ -2846,7 +2846,7 @@ static void file_xfer_init_task_async_cb(GObject
> > > *obj,
> > > GAsyncResult *res, gpoint
> > >          goto failed;
> > >  
> > >      channel = spice_file_transfer_task_get_channel(xfer_task);
> > > -    basename = g_file_info_get_attribute_as_string(info,
> > > G_FILE_ATTRIBUTE_STANDARD_NAME);
> > > +    basename = g_file_info_get_attribute_as_string(info,
> > > G_FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME);
> > >      file_size = g_file_info_get_attribute_uint64(info,
> > >      G_FILE_ATTRIBUTE_STANDARD_SIZE);
> > >  
> > >      xfer_op = data;
> > 
> > I would be curious to see different OSes with even extra plane 0
> > characters.
> > 
> > Frediano
>