Skip to content
Snippets Groups Projects

lua: discard basedir and DIR_SEP on UWP

Open Johannes Kauffmann requested to merge JohannesKauffmann/vlc2:fix-uwp-lua into master
1 unresolved thread

On UWP, the lua folder is deployed right next to the executable in the app installation folder, but the path returned by config_GetUserDir (which is basedir) points to the local packages folder, located in the current user AppData folder. Therefore, discard basedir and the directory separator, to allow VLC to find the lua folder.

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #153984 passed

Merge request pipeline passed for 7d9e7f13

Approval is optional
Merge blocked: 2 checks failed
Unresolved discussions must be resolved.
Merge request must be rebased, because a fast-forward merge is not possible.

Merge details

  • The source branch is 16117 commits behind the target branch.
  • 1 commit will be added to master.
  • Source branch will be deleted.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
183 183 if (unlikely(basedir == NULL))
184 184 return list;
185 185
186 #ifndef VLC_WINSTORE_APP
186 187 if (likely(asprintf(list, "%s"DIR_SEP"lua"DIR_SEP"%s",
187 188 basedir, luadirname) != -1))
189 #else
190 if (likely(asprintf(list, "lua"DIR_SEP"%s", luadirname) != -1))
  • This looks like reintroducing an old vulnerability where VLC loads security-critical assets from the CWD.

  • Then, there needs to be a way to get the absolute path to the folder with the UWP executable, to use that as basedir. So like @jbk said, I'd imagine that would need to be implemented in the core.

  • VLC_USERDATA_DIR corresponds to %AppData% on Windows and should not be changed (used by the MediaLibrary for example).

    We may need a separate VLC_XXX_DIR to address the internal scripts. None of these seems to fit:

    • VLC_HOME_DIR /* User's home */
    • VLC_CONFIG_DIR /* VLC-specific configuration directory */
    • VLC_USERDATA_DIR /* VLC-specific data directory */
    • VLC_CACHE_DIR /* VLC-specific user cached data directory */

    Then on UWP we can report a different location than the one from a regular Win32 installation (dirs.c vs dirs-uap.c).

  • I fail to see a problem with using the user data directory here. This is the directory in which to place generic stuff that:

    • cannot be regenerated on the fly: cache
    • and does not fit in any other category: configuration, downloads, templates, public share, documents, music, pictures, videos.

    AFAIK, no platform defines a more specific and thus better fitting directory. Adding a new script directory that just ends up aliasing the data directory on all platforms would not be terribly useful.

  • VLC_USERDATA_DIR is outside of the VLC "container" on UWP. Here we need the location of the lua folder which is inside that VLC installation "container" (package/folder/etc). None of the above points to that folder.

    What we need is something like VLC_SHARE_DIR.

  • The package folder for "official" installed Lua scripts is handled separately. This code snippet is for custom user-specific custom scripts.

  • This code is called for VLC_USERDATA_DIR, VLC_PKG_LIBEXEC_DIR and VLC_PKG_DATA_DIR.

    The share folder is actually added via config_GetSysPath(VLC_PKG_DATA_DIR, NULL). Maybe it doesn't point in the right place in UWP ? Or the UWP packaging doesn't put the lua scripts in a lua folder as expected ?

  • Official .lua and .luac should be in pkgdata and pkglibexec respectively

  • Please register or sign in to reply
  • added MRStatus::Stale label and removed MRStatus::InReview label

  • added MRStatus::InReview label and removed MRStatus::Stale label

  • added MRStatus::Stale label and removed MRStatus::InReview label

  • What can we do about this one?

  • added MRStatus::InReview label and removed MRStatus::Stale label

  • added MRStatus::Stale label and removed MRStatus::InReview label

  • changed milestone to %3.0.x maintenance

  • Steve Lhomme changed milestone to %4.0

    changed milestone to %4.0

  • Please register or sign in to reply
    Loading