Skip to content

screenshot: long filename handling improvements #10052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rtldg wants to merge 1 commit into from
Closed

screenshot: long filename handling improvements #10052

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions player/screenshot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,28 @@ static void append_filename(char **s, const char *f)
talloc_free(append);
}

// Screenshot base filenames (especially with %{sub-text}) can be longer than
// filesystems support (255 generally). Truncate it to fit with an extension.
// If truncation produces an invalid UTF-8 codepoint, then chop that off.
static void truncate_long_base_filename(char *s, const size_t ext_len)
{
const size_t max_utf8_bytes = 255 - (ext_len + 1); // ext_len+1 for '.'
Copy link
Member

@avih avih Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

255 is a magic number. It should not be hardcoded. It should probably be MAX_PATH.

On windows MAX_PATH can be more than 255, because it has enough space to hold UTF8 of 260 wchar_t elements.

Also, if extlen is 255 or more, then max_utf8_bytes will wrap around to a huge number...

Copy link
Contributor Author

@rtldg rtldg Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be MAX_PATH because MAX_PATH is not the basename length. MAX_PATH is to hold drive-letter + ":\" + basename of 255 256(??) wchar_t/char elements + NUL terminator.

Per Maximum Path Length Limitation for Windows:

This type of path is composed of components separated by backslashes, each up to the value returned in the lpMaximumComponentLength parameter of the GetVolumeInformation function (this value is commonly 255 characters).

Bothering to check GetVolumeInformation isn't worth doing though.
All relevant filesystem use 255 for segments of filename (including for non-Windows OSes).

If ext_len is 255 let mpv blow up because that's an absurd case to care about.

Copy link
Member

@avih avih Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be MAX_PATH

Which is why I said probably, I.e. you should figure whether it should be MAX_PATH or something else, like MAX_NAME.

The point is that 255 should not be hardcoded. It should be appropriate for the current platform, and if it's not MAX_PATH and not MAX_NAME then you should figure out what it needs to be, without calling APIs.

It should probably be some existing constant of the platform, and not hardcoded inside this function.

If ext_len is 255 let mpv blow up because that's an absurd case to care about.

In your applications maybe. Not in mpv.

You mean that the example you gave which should be fixed are not absurd cases, and so we should really care about them, like this?

screenshot-template="aπŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚"

Luckily for you though, it won't blow up, but it will also not work.

It's not rocket science. Please fix it correctly.

Copy link
Contributor Author

@rtldg rtldg Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is why I said probably, I.e. you should figure whether it should be MAX_PATH or something else, like MAX_NAME.

Which is what I did and why 255.

The point is that 255 should not be hardcoded. It should be appropriate for the current platform, and if it's not MAX_PATH and not MAX_NAME then you should figure out what it needs to be, without calling APIs.

Don't hardcode but also figure it out without calling APIs? Okay...

If ext_len is 255 let mpv blow up because that's an absurd case to care about.

In your applications maybe. Not in mpv.

The extension comes from mpv. If mpv decides to use 255 character long extensions then that is mpv's fault.

screenshot-template="aπŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚πŸŒ‚"

Luckily for you though, it won't blow up, but it will also not work.
It's not rocket science. Please fix it correctly.

You're missing something if you think that won't work, or why it was listed as an example for testing removal of invalidated UTF-8 codepoints due to truncation.

Copy link
Member

@avih avih Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably be some existing constant of the platform

I don't know if it's available.

We have this:

mpv/osdep/io.c

Lines 539 to 547 in ef4c6df

// Windows' MAX_PATH/PATH_MAX/FILENAME_MAX is fixed to 260, but this limit
// applies to unicode paths encoded with wchar_t (2 bytes on Windows). The UTF-8
// version could end up bigger in memory. In the worst case each wchar_t is
// encoded to 3 bytes in UTF-8, so in the worst case we have:
// wcslen(wpath) * 3 <= strlen(utf8path)
// Thus we need MP_PATH_MAX as the UTF-8/char version of PATH_MAX.
// Also make sure there's free space for the terminating \0.
// (For codepoints encoded as UTF-16 surrogate pairs, UTF-8 has the same length.)
#define MP_PATH_MAX (FILENAME_MAX * 3 + 1)

But it's only used privately at this C file when enumerating files in a directory.

Not sure how to solve this in general. I don't think we should change the global MAX_PATH either.

Copy link
Contributor Author

@rtldg rtldg Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAME_MAX seems to available in limits.h for Linux/macos as 255. It's also in my mingw64/msys2's limits.h but behind a _POSIX_ ifdef. BSDs have MAXNAMELEN which is 255 as far as I can tell.

Something like this, hardcoding 255, or calling out to GetVolumeInformation/pathconf(_PC_NAME_MAX)

#include <limits.h>
#ifndef NAME_MAX
 #ifdef MAXNAMELEN
  #define NAME_MAX MAXNAMELEN
 #else
  #define NAME_MAX 255
 #endif
#endif

Copy link
Member

@kasper93 kasper93 Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For win32 you can use _MAX_FNAME https://learn.microsoft.com/en-us/cpp/c-runtime-library/path-field-limits

Note that _MAX_FNAME includes space for terminating null, while NAME_MAX does not.

EDIT: And just as I mentioned in the other PR. Shouldn't we set long paths support in manifest on Windows?

Copy link
Contributor Author

@rtldg rtldg Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that _MAX_FNAME includes space for terminating null, while NAME_MAX does not.

I didn't include it because of that but yeah a Windows specific define could be (_MAX_FNAME-1) which is 255

Copy link
Contributor

@hrxn hrxn Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: And just as I mentioned in the other PR. Shouldn't we set long paths support in manifest on Windows?

In my opinion, yes..

char *basename = mp_basename(s);
size_t len = strlen(basename);

if (len <= max_utf8_bytes)
return;

basename[max_utf8_bytes] = 0;

for (int i = 1; i <= 3; i++) {
if (bstr_parse_utf8_code_length(basename[max_utf8_bytes-i]) > i) {
basename[max_utf8_bytes-i] = 0;
break;
}
}
}

static char *create_fname(struct MPContext *mpctx, char *template,
const char *file_ext, int *sequence, int *frameno)
{
Expand Down Expand Up @@ -263,6 +285,7 @@ static char *create_fname(struct MPContext *mpctx, char *template,
}

res = talloc_strdup_append(res, template);
truncate_long_base_filename(res, strlen(file_ext));
res = talloc_asprintf_append(res, ".%s", file_ext);
char *fname = mp_get_user_path(NULL, mpctx->global, res);
talloc_free(res);
Expand Down