Skip to content
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.

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

Improve setting out-of-range values in mac implementation #28

Merged
merged 9 commits into from
Mar 10, 2023

Conversation

infeo
Copy link
Member

@infeo infeo commented Mar 6, 2023

Update: Replace exceptions with max_value write

Summary

Extend mac implemenation to support up to unsigned integer values correctly and for greater values always report MAX_UINT instead of throwing exception.

Description

jfuse supports different operating systems and process architectures with a single API. Hence, if there are differences between systems/architectures, we have to choose the most generic one in the API and have to adapt to such differences in the API implementations.

An example is the Statvfs interface with the method void setBlocks(long blocks);. As a parameter it accepts a long value. But on macOS actuall only values up to unsigned integers are supported, since the system structure statvfs has only this size:

/*
 * sys/statvfs.h
 */
#ifndef _SYS_STATVFS_H_
#define	_SYS_STATVFS_H_

#include <sys/_types.h>
#include <sys/cdefs.h>

#include <sys/_types/_fsblkcnt_t.h>
#include <sys/_types/_fsfilcnt_t.h>

/* Following structure is used as a statvfs/fstatvfs function parameter */
struct statvfs {
	unsigned long	f_bsize;	/* File system block size */
	unsigned long	f_frsize;	/* Fundamental file system block size */
	fsblkcnt_t	f_blocks;	/* Blocks on FS in units of f_frsize */
	fsblkcnt_t	f_bfree;	/* Free blocks */
	fsblkcnt_t	f_bavail;	/* Blocks available to non-root */
	fsfilcnt_t	f_files;	/* Total inodes */
	fsfilcnt_t	f_ffree;	/* Free inodes */
	fsfilcnt_t	f_favail;	/* Free inodes for non-root */
	unsigned long	f_fsid;		/* Filesystem ID */
	unsigned long	f_flag;		/* Bit mask of values */
	unsigned long	f_namemax;	/* Max file name length */
};

with typedef unsigned int __darwin_fsblkcnt_t; /* Used by statvfs and fstatvfs */.

Our current adaption allows values up to Java's Integer.MAX_VALUE:

@Override
public void setBlocks(long blocks) {
if (blocks > Integer.MAX_VALUE) {
throw new IllegalArgumentException("Max supported number of blocks: " + Integer.MAX_VALUE);
} else {
statvfs.f_blocks$set(segment, (int) blocks);
}
}

But since Java integers are signed, we are unnecessarily narrowing the allowed values to 2147483647, even thou an unsigned int can have values up to 4294967295.

To make matters worse, we throw an unchecked exception if the value is out of range, making it impossible to report any (maybe reasonable) value.

This PR changes two things:

  • widen the supported range to unsigned integers
  • if value is still out of range, just report the biggest unsinged integer

@infeo infeo added the enhancement New feature or request label Mar 6, 2023
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

The getters should be fine, I guess. statvfs uses unsigned ints on C side and casting int32 to uint64 is lossless.

// sys/_types.h:
typedef unsigned int	__darwin_fsblkcnt_t;	/* Used by statvfs and fstatvfs */

// statvfs.h:
typedef __darwin_fsblkcnt_t	fsblkcnt_t;
// ...
fsblkcnt_t	f_blocks;	/* Blocks on FS in units of f_frsize */
fsblkcnt_t	f_bfree;	/* Free blocks */
fsblkcnt_t	f_bavail;	/* Blocks available to non-root */

The setter are difficult, though. We currently have a related bug (cryptomator/cryptomator#2760) that would not be fixed by allowing merely twice as many blocks (uint32 instead of int32).

Instead I would suggest we investigate whether it does any harm to report a wrong number of blocks: Instead of throwing an exception, just cap the value. E.g. imagine there are 0x0000 F000 0000 0000 blocks available, we (incorrectly) report this as 0xFFFF FFFF. While VFS size is now incorrect, the user has sufficient space to work on the volume. For any value that fits into a 32bit int, we use the correct value. So as soon as there is a shortage of storage space, it will report correctly, blocking attempts to write beyond capacity.

@infeo infeo changed the title allow up to unsigned int maximum when setting values in statvfs Improve setting out-of-range values in macOS Mar 10, 2023
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

Can you add tests, in which you pass 0x01234567_89abcdefL to the setters and verify, the memory contains 0xFFFFFFFF?

@infeo infeo changed the title Improve setting out-of-range values in macOS Improve setting out-of-range values in mac implementation Mar 10, 2023
@infeo infeo merged commit b883182 into develop Mar 10, 2023
@infeo infeo deleted the feature/mac-accept-unsigned-values branch March 10, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants