document-portal: use default FUSE implementation for statfs

[SlonkeyMaster]

For a little shy of 5 years, the xdg-document-portal implementation of the statfs syscall has been broken and returned EPERM. The default FUSE implementation returns a sensible default value, similar to the /proc and /sys mount points. This merge request comments out the broken implementation in favor of FUSE's default, and fixes #553.

I understand this is not an ideal implementation of this FUSE interface. On the other hand, this change breaks no test, and allows df -a (and any other program using the statfs syscall) to run in harmony with xdg-document-portal.

[smcv]

What information does the default implementation provide?

If fuse's default implementation does everything we need, then please delete this instead of commenting it out, and delete the now-unused implementation of xdp_fuse_statfs(). There's no point in keeping dead code in-tree, it's just a source of compiler warnings: we can always get it back from the git history if we need it.

[smcv]

But... would the default implementation allow a sandboxed app to distinguish between the existence and nonexistence of a document that it does not have access to? If it would, then using the default implementation would be an information disclosure vulnerability.

[smcv]

the xdg-document-portal implementation of the statfs syscall has been broken

What is broken about it is a more interesting fact.

Looking at that implementation, it seems to be assuming that it's being called for an inode that represents a document, for which the safe fallback is to fail with EPERM to avoid leaking information that the caller shouldn't have been allowed to know about, like the existence or nonexistence of a particular file.

In some of the other interfaces likexdp_fuse_getattr() and xdp_fuse_opendir(), a distinction is made between "virtual directories" and "documents", via xdp_domain_is_virtual_type().

Probably the right logic for xdp_fuse_statfs() would be something like this (untested pseudocode based on xdp_fuse_opendir() and xdp_fuse_getattr()):

  XdpDomain *domain = inode->domain;                                              

  if (xdp_domain_is_virtual_type (domain))                                        
    {
      struct statvfs buf;
      ... fill in buf with dummy data similar to FUSE's default implementation ...
      fuse_reply_statfs (req, &buf);
      return;
    }

... else continue with current implementation ...

[SlonkeyMaster]

The default implementation of fuse uses memset too to create the dummy data for statfs, so this is concordant, and doesn't break other userspace tools. This should fix #553.

[smcv]

The default implementation of fuse uses memset too to create the dummy data for statfs

I think you might be reading the wrong part of libfuse: do_statfs() in lib/fuse_lowlevel.c looks like it's the default implementation that would be used if we didn't supply one, and it fills in f_namemax = 255 and f_bsize = 512. I assume there must be some compatibility reason why it needs to fill those; if yes, we should probably do the same.

I'd be inclined to initialize buf like libfuse does, rather than memset'ing it and then filling in certain members - that lets the compiler choose whatever is the most optimal way to achieve the desired result.

[smcv]

Please indent consistently with the surrounding code:

Suggested change

	if(xdp_domain_is_virtual_type(inode->domain)) {
	memset(&buf, 0, sizeof(buf));
	fuse_reply_statfs (req, &buf);
	return;
	}
	if (xdp_domain_is_virtual_type (inode->domain))
	{
	memset (&buf, 0, sizeof (buf));
	fuse_reply_statfs (req, &buf);
	return;
	}

[smcv]

I think this deserves a comment as to why this is appropriate, perhaps something like:

Suggested change

	/* Free space, etc. don't make much sense for a virtual filesystem,
	* so reply with a mostly empty buffer. This is consistent with what libfuse does
	* if we don't supply a statfs implementation. */

[smcv]

(but obviously that comment is misleading if what we do here is not consistent with what libfuse would have done without a statfs implementation!)

[swick]

Hey @SlonkeyMaster, are you going to pick this PR up again?