From CI run logs:

In file included from /home/runner/work/physfs/physfs/src/physfs.c:12:
/home/runner/work/physfs/physfs/src/physfs.c: In function ‘openDirectory’:
/home/runner/work/physfs/physfs/src/physfs.c:929:40: warning: ?: using integer constants in boolean context [-Wint-in-bool-context]
  929 |     BAIL_IF(!retval, claimed ? errcode : PHYSFS_ERR_UNSUPPORTED, NULL);
      |                      ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/physfs/physfs/src/physfs_internal.h:273:44: note: in definition of macro ‘BAIL_IF’
  273 | #define BAIL_IF(c, e, r) do { if (c) { if (e) PHYSFS_setErrorCode(e); return r; } } while (0)
      |                                            ^

Is the following correct??

diff --git a/src/physfs.c b/src/physfs.c
index e7ceddd..568f1fc 100644
--- a/src/physfs.c
+++ b/src/physfs.c
@@ -921,12 +921,12 @@ static DirHandle *openDirectory(PHYSFS_Io *io, const char *d, int forWriting)
             retval = tryOpenDir(io, *i, d, forWriting, &claimed);
     } /* else */
 
-    errcode = currentErrorCode();
+    errcode = claimed ? currentErrorCode() : PHYSFS_ERR_UNSUPPORTED;
 
     if ((!retval) && (created_io))
         io->destroy(io);
 
-    BAIL_IF(!retval, claimed ? errcode : PHYSFS_ERR_UNSUPPORTED, NULL);
+    BAIL_IF(!retval, errcode, NULL);
     return retval;
 } /* openDirectory */
 

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Fix read error checking (physfs-iso9660-fix-read-error-checking.diff, text/plain, 2015-04-10 12:28:41 -0400, 1293 bytes)
• Fix magic memcmp length (physfs-iso9660-compare-5-bytes-of-magic.diff, text/plain, 2015-04-10 12:29:13 -0400, 841 bytes)

Reported in version: all Reported for operating system, platform: Linux, PC

Comments on the original bug report:

On 2015-04-10 12:28:41 -0400, Jonathan Hamilton wrote:

Created attachment 3530 Fix read error checking

As far as I can see, iso_readimage() and io->read() return the number of bytes read, but this seems wrong in a couple of places:

• Reading 5 bytes of the magic number appears to expect io->read() to return 'Not 5'. This value is then checked with a memcmp of 6 bytes, comparing 1-over the array.
• Reading the file descriptor appears to expect iso_readimage to return '1' on success, however it actually returns the number of bytes read.

Patches should be attached.

Thanks, Jonathan

On 2015-04-10 12:29:13 -0400, Jonathan Hamilton wrote:

Created attachment 3531 Fix magic memcmp length

Hi!

A very discrete problem: i have a zip of an dos game. This is mounted via dosboxx via physfs overlay. In this archive there is an file "sndcfg.ini". I need to replace this file while using the overlay. This is not possible. This the old file from the archive is used, although there is the new sndcfg.ini in the overlay part.

wish: in doubt use the overlays file (when its newer etc)

marco

Hello, I would like to clarify whether it is possible to create your own archive format using the library?

If so, could you briefly explain what procedures need to be performed?

My use case is an archiver that reads the entire container file into memory in openArchive and serves chunks of memory in openRead by using createMemoryIo; attached test is reduced (so appears nonsensical)

The core problem are the different semantics of the ATOMIC_DECR impls https://github.com/icculus/physfs/blob/main/src/physfs_internal.h#L116 ff:

• _InterlockedDecrement : https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockeddecrement

The function returns the resulting decremented value.

• __sync_fetch_and_add : https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html

and returns the value that had previously been in memory

In https://github.com/icculus/physfs/blob/main/src/physfs.c#L356

    assert(info->refcount > 0);  /* even in a race, we hold a reference. */

    if (__PHYSFS_ATOMIC_DECR(&info->refcount) == 0)

If refcount > 0 and the impl is __sync_fetch_and_add(ptrval, -1) the return value will still be > 0 and the cleanup block is never used.

The fallback impl also returns the unmodified value; haven't looked at the watcom asm. note: ATOMIC_INCR has the same problem but the return value is never used.

valgrind --leak-check=full --show-reachable=yes ./a.out
…
==862567== 6 bytes in 1 blocks are indirectly lost in loss record 1 of 3
==862567==    at 0x483877F: malloc (vg_replace_malloc.c:307)
==862567==    by 0x1130EF: mallocAllocatorMalloc (physfs.c:3248)
==862567==    by 0x10C531: ABC_openRead (test_memio.c:36)
==862567==    by 0x112112: PHYSFS_openRead (physfs.c:2767)
==862567==    by 0x10C6DA: main (test_memio.c:111)
==862567== 
==862567== 48 bytes in 1 blocks are indirectly lost in loss record 2 of 3
==862567==    at 0x483877F: malloc (vg_replace_malloc.c:307)
==862567==    by 0x1130EF: mallocAllocatorMalloc (physfs.c:3248)
==862567==    by 0x10D051: __PHYSFS_createMemoryIo (physfs.c:392)
==862567==    by 0x10C560: ABC_openRead (test_memio.c:38)
==862567==    by 0x112112: PHYSFS_openRead (physfs.c:2767)
==862567==    by 0x10C6DA: main (test_memio.c:111)
==862567== 
==862567== 134 (80 direct, 54 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==862567==    at 0x483877F: malloc (vg_replace_malloc.c:307)
==862567==    by 0x1130EF: mallocAllocatorMalloc (physfs.c:3248)
==862567==    by 0x10D029: __PHYSFS_createMemoryIo (physfs.c:390)
==862567==    by 0x10C560: ABC_openRead (test_memio.c:38)
==862567==    by 0x112112: PHYSFS_openRead (physfs.c:2767)
==862567==    by 0x10C6DA: main (test_memio.c:111)

Those leaks go away when replacing __sync_fetch_and_add with __sync_add_and_fetch. test_memio.c.txt

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Fix ISO9660FileFlags struct size on msvc (physfs-iso9660-fix-ISO9660FileFlags-size-in-msvc.diff, text/plain, 2015-04-10 12:34:56 -0400, 1163 bytes)

Reported in version: all Reported for operating system, platform: Windows Vista, PC

Comments on the original bug report:

On 2015-04-10 12:34:56 -0400, Jonathan Hamilton wrote:

Created attachment 3532 Fix ISO9660FileFlags struct size on msvc

MSVC (at least version 13 - 'VS Express 2013 for Desktop') appear to expect the minimum size of packed bitfields to be specified. This means using 'unsigned' causes the size of the flags field to be 4 bytes (instead of the expected one).

This then causes the sizeof() the various record structs to be incorrect, causing them to be incorrectly read from the file.

Tested on windows 7 and windows 8.1 built with 'VS Express 2013 for Desktop'

Converting UTF-16 surrogate pairs to temporary UTF-32 codepoints is missing the 0x10000 bit, which causes failure when enumerating directories containing particular unicode filenames on Windows.

The CMake build currently uses include_directories() to add the src directory to the includes. It would be better to use target_include_directories() to have the target include the actual directory rather than in the global cmake space.

https://cmake.org/cmake/help/latest/command/target_include_directories.html

option(PHYSFS_BUILD_STATIC "Build static library" TRUE)
if(PHYSFS_BUILD_STATIC)
    add_library(physfs-static STATIC ${PHYSFS_SRCS})
    target_include_directories(physfs-static PUBLIC src)

I'm calling PHYSFS_stat() on a directory contained in the root, and with a single ZIP mounted and a "root directory" set to "data"

In function verifyPath(), the "prepend the root directory, if any" section, fname[] is being set to "dat/dir" rather than "data/dir". The issue seems to be an off-by-one, when prepending the root directory.

Old code:

if (h->root)
{
    const int isempty = (*fname == '\0');
    fname -= h->rootlen - 1;
    strcpy(fname, h->root);
    if (!isempty)
        fname[h->rootlen - 2] = '/';
    *_fname = fname;
} /* if */

Modified code:

if (h->root)
{
    const int isempty = (*fname == '\0');
    fname -= h->rootlen;
    strcpy(fname, h->root);
    if (!isempty)
        fname[h->rootlen - 1] = '/';
    *_fname = fname;
} /* if */

This seems to fix the problem, but I'm not sure if it could introduce problems elsewhere.

1. Bumping CMake minimum version to 3.0 gets rid of these warnings:

   CMake Deprecation Warning at CMakeLists.txt:12 (cmake_minimum_required):
     Compatibility with CMake < 2.8.12 will be removed from a future version of
     CMake.
   
     Update the VERSION argument <min> value or use a ...<max> suffix to tell
     CMake that the project does not need compatibility with older versions.
   

   and

   CMake Warning (dev) at CMakeLists.txt:15 (project):
     Policy CMP0048 is not set: project() command manages VERSION variables.
     Run "cmake --help-policy CMP0048" for policy details.  Use the cmake_policy
     command to set the policy and suppress this warning.
   
     The following variable(s) would be set to empty:
   
       CMAKE_PROJECT_VERSION
       CMAKE_PROJECT_VERSION_MAJOR
       CMAKE_PROJECT_VERSION_MINOR
       CMAKE_PROJECT_VERSION_PATCH
   This warning is for project developers.  Use -Wno-dev to suppress it.
   

2. use list(APPEND) where applicable.
3. use WIN32. Also, the comment "we're about to move to Win64" is outdated.
4. conditionally enable c++
5. Create PhysFS::PhysFS and PhysFS::PhysFS-static targets, and export those via the installed cmake packages.
6. Use target_include_directories(tgt PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}") This fixes https://github.com/icculus/physfs/issues/14 (You need to use a generator expression because you don't want to make build machine paths to appear in the exported cmake package)

Fixes https://github.com/icculus/physfs/issues/14

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Simple Android.mk file. Enables ZIP and APK support. (Android.mk, text/x-makefile, 2014-04-27 18:48:46 -0400, 585 bytes)
• Patch with the implementation and the required changes for src/physfs.c and the CMake build script (physfs_apk_support.patch, text/plain, 2014-04-27 18:50:33 -0400, 4951 bytes)

Reported in version: all Reported for operating system, platform: other, Other

Comments on the original bug report:

On 2014-04-27 18:48:46 -0400, rettichschnidi wrote:

Created attachment 3439 Simple Android.mk file. Enables ZIP and APK support.

Based on the archiver_zip.c file I created a super simple hack which allows to use physfs to mount an Android APK file.

It basicly just prexfixes each filename with "/assets/" and then forwards the real work to the ZIP implementation in archiver_zip.c.

There is also a simple Android.mk file which is configured to enable ZIP and APK support.

On 2014-04-27 18:50:33 -0400, rettichschnidi wrote:

Created attachment 3440 Patch with the implementation and the required changes for src/physfs.c and the CMake build script

I am looking into using PhysFS on an open-source game engine re-implementation and I was putting together a small test program that was supposed to be able to mount an ISO file (the original game data), a ZIP archive (a MOD) and a directory (user-provided resources), using the latest PhysFS release - 3.2.0 at the time of this writing.

Mounting the ZIP file and the directory is successful, however, mounting the ISO fails with error code 18: corrupted.

This is identical with the issue encountered by the OpenApoc project, and they "fixed" by patching PhysFS itself: https://github.com/JonnyH/physfs-hg-import/commit/ef1c6abc1c85a459f4ec721ff0cf6ba7b812b9f0

I've asked the developer that pushed the "fix" about fixing it upstream and he told me this:

I didn't really root-cause it, I suspect it's related to iso "formats" being a loose collection of slightly-incompatible extensions from different vendors - there's like 30 different ways of supporting longer filenames and different collections of file attributes.

If I apply that change to the PhysFS code the ISO seems to mount (and be parsed/files read) just fine.

Is that "fix" valid? Is there a way to make PhysFS ignore such errors already?

I have attached my test program, it includes my code, the PhysFS code, and the ISO, ZIP and data directories. Some notes are in the README.txt file found in the archive.

physfs-test.zip.

Currently projects using CPM that use this library will have their RPATH broken for all targets defined after the CPMAddPackage for physfs, unless CMAKE_SKIP_RPATH is force-set to OFF.

Example:

CPMAddPackage(
  NAME physfs
  GIT_REPOSITORY https://github.com/icculus/physfs
  GIT_TAG release-3.2.0
  OPTIONS "CMAKE_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX}"
                  "PHYSFS_ARCHIVE_GRP OFF"
		  "PHYSFS_ARCHIVE_WAD OFF"
		  "PHYSFS_ARCHIVE_HOG OFF"
		  "PHYSFS_ARCHIVE_MVL OFF"
		  "PHYSFS_ARCHIVE_QPAK OFF"
		  "PHYSFS_ARCHIVE_SLB OFF"
		  "PHYSFS_ARCHIVE_ISO9660 OFF"
		  "PHYSFS_ARCHIVE_VDF OFF")

# Cleanup after physfs' use of set to avoid breaking the linker.
if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID MATCHES "Clang")
  set(CMAKE_SKIP_RPATH OFF CACHE BOOL "Skip RPATH" FORCE)
endif()

Having an option to turn this behaviour off would be appreciated.

I can provide a small pull request that implements this, if this feature is accepted.

Changed encoding type from ASCII to Windows 125x for filenames in VDF. Right now some vdf files are not loaded, because some characters in filenames exeed 127 limit.

While porting physfs to be usable on Wii U homebrew, it was discovered this leaks a stack pointer out of scope.

We found this because PHYSFS_exists was failing, so the parameters of createDirHandle were logged:

00;41;58;987: mountPoint pêÀT¸T/
00;41;58;987: dirName fs:/vol/external01/save/game

We have patched this to make the minimum for smallAlloc be zero to avoid this issue. Hopefully this is a trivial fix.

This is preliminary, but here's some initial wishlist things I'd like to do:

• Make the Android stuff not use PHYSFS_Init with a magic structure. This was a wrong move.
• Move all the global state into a struct and let people create instances, each with their own search path, write dir, mutexes, etc. All the functions will take an instance pointer; for people that don't care about this or want to build legacy code, we'll leave the original entry points alone, and those will manage an internal instance (or write a physfs3-compat library).
• Remove CD-ROM support (entry point will remain but never find any discs).
• Multiple write directories; they'll interpolate and mount and such like the search path does.
• Queries for other writeable directories (config/cache/etc), instead of just a "pref path"
• Probably other things.

I contribute to the LÖVE game framework, which uses PhysFS, and a common user complaint has been that while the write-to-single-common-directory paradigm works OK for most games, it's too limited for things like tools and non-standard games or other apps that need to interact with the filesystem in more arbitrary manners.

Another related issue is that many operating systems (e.g. macOS, iOS, Windows) really want apps to use different common data locations for slightly different purposes. For example on iOS, files saved to Documents and Application Support may be automatically cloud-synced whereas files saved to tmp and Caches will not be. And Documents is meant for user-visible content (e.g. screenshots) whereas Application Support is not (e.g. a progress save file). Since iOS has restrictive sandboxing, I don't believe setting / as the write directory works.

In my opinion having to change the write directory - and having to make sure all open-for-write files are closed before doing so - each time a file is written to a different one of those locations is much too cumbersome and limiting, and doesn't fit with PhysFS' other virtual filesystem APIs.

To lift those limitations in LÖVE, I forked PhysFS and added a new PHYSFS_mountRW function which is just like PHYSFS_mount but with some internals changed to allow writing to the given location. Since write access tends to be allowed in fewer locations than read access, having a separate function from PHYSFS_mount still makes a lot of sense to me (although I suppose an enum or boolean parameter would work too, but it would also be a breaking change).

Here is my branch with the change: https://github.com/icculus/physfs/compare/main...slime73:mountRW It's been tested but not by a ton of people yet, and I don't have a ton of experience with PhysFS' codebase so there might be bugs.

My questions are: is the overall idea within the scope of what you want PhysFS to be, and if so is my API reasonable? If yes I can open a pull request. I don't mind maintaining a fork but I'd prefer having the idea upstreamed.