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

Allow reading empty files. #13

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Allow reading empty files. #13

merged 1 commit into from
Jul 14, 2020

Conversation

pteromys
Copy link
Contributor

@pteromys pteromys commented Jul 3, 2020

Symptom

cat: '/mnt/usb/empty.txt': Operation not permitted

Cause

The outputs of hfslib_get_file_extents() cannot distinguish between errors and an empty list of extents—in both cases, it sets *out_extents = NULL and returns 0 for the number of extents. Naturally, hfslib_readd_with_extents() returns -1 in this error case.

Fix

In hfsfuse_read(), don't call hfslib_readd_with_extents() if the file size is zero. This requires recording, in struct hf_file, either the file size or a boolean flag that the file is empty.

I went with a boolean flag in a uint8_t to be nicer to memory-constrained users, and to make it clear that we don't actually use the full file size information. But I didn't really think about performance at all.

@0x09
Copy link
Owner

0x09 commented Jul 3, 2020

Thanks, looks great. I was curious if it made sense to just return EOFs any time the extents were empty, which AFAIK outside of this scenario could only occur as a result of corruption, so I decided to check out how the Apple driver handles this and other mismatches between logical/extent size by tweaking a test image.

Looking at the fork structure with extents embedded there are essentially 3 values in play: the reported logical size, the total number of blocks worth of extents, and the block count per extent.

struct hfs_fork_t {
	uint64_t logical_size;
	uint32_t clump_size;
	uint32_t total_blocks;
	struct {
		uint32_t start_block;
		uint32_t block_count;
	} extents[8];
};

The driver takes total_blocks quite seriously -- any time this (x the block size) is less than the logical_size open itself will fail with EINVAL.
As long as total_blocks is greater than or equal to the logical size then open will succeed and it's not used further. The sum of the block_counts isn't taken into consideration there but it is used for reading. Both read and stat will actually report/give the minimum of the logical_size and the block_count (x the block size).

Point being that referencing the logical size like you're doing here for is definitely more correct according to the official driver than my idea.

n.b. It's not particularly important that we handle random fork data corruption identically to the official driver (although I may try to tweak some things knowing that.)

src/hfsfuse.c Outdated
@@ -48,6 +48,7 @@ struct hf_file {
hfs_extent_descriptor_t* extents;
uint16_t nextents;
uint8_t fork;
uint8_t is_empty;
Copy link
Owner

Choose a reason for hiding this comment

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

Even though these are almost certainly typedef'd to the same thing, can you add stdbool to the other stdlib includes in this file and use the actual bool type for this?
We target C11 so don't hesitate to use newer stuff in the codebase as long as it's not an extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!—done and squashed.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you!

Symptom:
cat: '/mnt/usb/empty.txt': Operation not permitted

Cause:
The outputs of hfslib_get_file_extents() cannot distinguish between
errors and an empty list of extents---in both cases, it sets
*out_extents = NULL and returns 0 for the number of extents.
Naturally, hfslib_readd_with_extents() returns -1 in this error case.

Fix:
In hfsfuse_read(), don't call hfslib_readd_with_extents() if the file
size is zero. This requires recording, in struct hf_file, either the
file size or a boolean flag that the file is empty.
@pteromys
Copy link
Contributor Author

Huh, good to know. Thanks for digging into that! All I was going off of was a suspicion that silently swallowing the symptoms of corruption would be wrong.

@0x09 0x09 merged commit 9509a5d into 0x09:master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants