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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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

I am pretty sure we agreed on utf8, but I can't find a reference either (it's already quite old!)

> 
> > 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() ].

interesting! I suspect it worked better before the change (in local encoding), but was still buggy in various cases ;).

My concern with DISPLAY_NAME is that perhaps some files don't have a displayable name?

> 
> 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.
> 
>     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
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>