Re: [FFmpeg-devel] [PATCH v2 1/2] avutil/wchar_filename, file_open: Support long file names on Windows

[Soft Works <softworkz@hotmail.com>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of nil-
> admirari@mailo.com
> Sent: Monday, May 16, 2022 10:12 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] avutil/wchar_filename,
> file_open: Support long file names on Windows
> 
> > +static inline int path_is_extended(const wchar_t *path)
> > +{
> > + size_t len = wcslen(path);
> > + if (len >= 4 && path[0] == L'\\' && (path[1] == L'\\' || path[1]
> == L'?') && path[2] == L'?' && path[3] == L'\\')
> 
> Length check is probably unnecessary: comparisons will reject '\0'
> and further comparisons won't run due to short-circuiting.

Yup, I think you're right, even though it would appear "unsafe" at
first sight. I've removed the length check.


> > + // The length of unc_prefix is 6 plus 1 for terminating zeros
> > + temp_w = (wchar_t *)av_calloc(len + 6 + 1, sizeof(wchar_t));
> 
> Not really true. The length of unc_prefix is 8.
> 2 is subtracted because UNC path already has \\ at the beginning.

Correct. It actually needs to be "len - 2 + 8 + 1".
I've updated the comment and the calculation.


> > + if (len >= 260 || (*ppath_w)[len - 1] == L' ' || (*ppath_w)[len -
> 1] == L'.') {
> 
> 1. Please change 260 to MAX_PATH.

Done.


> 2. GetFullPathName removes trailing spaces and dots, so the second
> part is always false.

Yea, when someone would want to handle such (weird) kind of path,
one would need to specify an extended path directly.
Removed the second part.


> > We already have win32_stat, but what's a bit tricky is that the
> > struct that this function takes as a parameter is named the same
> > as the function itself.
> 
> Sorry, I thought is was a definition of a function, not a struct.
> Since stat function is already defined as win32_stat,
> It's better to revert the changes.

stat wasn't already defined as win32_stat.
win32_stat was already defined but not mapped. That's what my change
does. 


> > The functions are needed in both. file_open.c cannot be included
> > in libavformat/os_support.h and neither the other way round,
> > so they need to be in a 3rd place. How about renaming
> > wchar_filename.h to windows_filename.h ?
> 
> Probably it's better to rename.

OK, but let's do this in subsequent patch shortly after.


> > I have skipped those checks because we won't have partially
> qualified
> > paths at this point (due to having called GetFullPathNameW) and
> > device paths are not allowed to be longer than 260, so this it might
> > happen that the UNC prefix gets added, but only when it's a long
> > path which doesn't work anyway (I've tested those cases).
> 
> I think it's better to test for \\.\ explicitly in path_is_extended:
> 1. It's not obvious that \\.\ aren't allowed to be long.
> 2. Probably FFmpeg is not going to have a longPathAware manifest,
>    but it can be linked with an EXE with such a manifest.
>    Would MAX_PATH restriction still apply?

That's a good question, but we need to be clear that device paths are
actually intended for accessing devices, e.g. like \\.\COM1
The fact that the prefix also works with a drive-letter path
is more due to some heritage and nobody would normally want to 
use such kind of paths to access files.

That being said, I've added a check (path_is_device_path()) for 
this now in the same way as .NET is doing it - which means
inside add_extended_prefix().


> You have the checks inside of get_extended_win32_path and none
> inside of add_extended_prefix. Yet add_extended_prefix can be called
> by anyone: it's not private. Thus add_extended_prefix either should be
> inlined,
> or it should have the necessary checks in place. Otherwise you end up
> with
> an API that's easy to use incorrectly and hard to use correctly, and
> it should be the other way around.

I have added checks there now for both device path and extended path
prefix. I have also clarified the function doc even further, so I 
hope it's sufficiently clear now. ;-)


> > And what's the point about this?
> 
> Point is obvious: extended paths are difficult to handle correctly.
> get_extended_win32_path cannot be used on its own, only as a final
> step before getting FILE* or a file descriptor.

Yes, that's how it's meant to be used, so the whole application can
be kept free from dealing with it.


Thanks again for the review, these were some good improvements.

Kind regards,
softworkz




_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".