Created attachment 357450 [details] [review]
Guard when adding or multiplying

Here's an initial version.

It adds two new utility macros: ADD_AND_CLAMP and MULTIPLY_AND_CLAMP that do safe additions and multiplications.

I patched several backends to use them. I only looked at backends that were doing such calculations in query_fs_info calls. I didn't guard normal query_info calls, since I figured one file was less likely to overflow.

With this patch, my webdav server now replies with:
  filesystem::size: 18446744073709551615
  filesystem::free: 18446744073709551613
  filesystem::type: webdav
  filesystem::remote: TRUE
  filesystem::used: 6036056
  gvfs::backend: dav

Which is much more correct.  This is the only backend I manually tested.

There is an argument that maybe we should not reply with a given attribute if it overflows (and presume that a client will treat that as infinite, or at least "big enough").  I'm happy to go back and try that approach instead, but it would complicate the code a fair bit more.

Review of attachment 357450 [details] [review]:

Thanks for your another contribution. But to be honest, I am not sure how to handle such values. The value you obtained from webdav probably means unlimited, but I wonder why it is not 2^64. But webdav specification is unfortunately pretty benevolent here. Not sure it makes sense to use those checks for all those backends. If such values are returned, they probably mean some kind of error, or infinity and thus should be handled differently and I would rather unset them in such cases...

The dav backend is probably the only one, where we calculates total size from used + free. Which is risky, when some service return 2^64 or something like that for unlimited, but in other cases is total size probably known and the overflow hazart is pretty negligible. So I would probably suggest to special case this in dav and do not care about the rest probably (and also the suggested fix for fuse is probably good idea). What do you think?

::: client/gvfsfusedaemon.c
@@ +648,3 @@
+            {
+              guint64 size = g_file_info_get_attribute_uint64 (file_info, G_FILE_ATTRIBUTE_FILESYSTEM_SIZE);
+              stbuf->f_blocks = ADD_AND_CLAMP (size, 4096 - 1, G_MAXUINT64) / 4096;

I would rather change the logic here to ((size > 0) ? ((size - 1) / 4096 + 1) : 0) and avoid the macros (hope this is equal).

::: common/gvfsutils.h
@@ +35,3 @@
+/* Macro used in backends to avoid overflows */
+#define ADD_AND_CLAMP(a, b, max) ((max) - (a) < (b) ? (max) : (a) + (b))
+#define MULTIPLY_AND_CLAMP(a, b, max) ((a) ? ((max) / (a) < (b) ? (max) : (a) * (b)) : 0)

I know that this is macro, but it is not for one file only, so I would use lower case and add gvfs_  prefix, e.g. gvfs_add_and_clamp

::: daemon/gvfsbackenddav.c
@@ +1206,3 @@
         g_file_info_set_attribute_uint64 (info,
                                          G_FILE_ATTRIBUTE_FILESYSTEM_SIZE,
+                                         ADD_AND_CLAMP(bytes_avail, bytes_used, G_MAXUINT64));

I would let this unset here, because the following from your example doesn't make much sense anyway, because used + free != size.

filesystem::size: 18446744073709551615
filesystem::free: 18446744073709551613
filesystem::used: 6036056

::: daemon/gvfsbackendgphoto2.c
@@ +2100,3 @@
             {
               g_mutex_lock (&gphoto2_backend->lock);
+              /* Clamp to int64 not uint64 since we treat negative values specially */

We have to be consistent and use uint64, so the code needs to be rewritten. The cache may hold prefilled GFileInfo instead and check for NULL for example. But I don't expect that gphoto2 device would ever return such big numbers...

::: daemon/gvfsbackendmtp.c
@@ +1179,3 @@
   for (storage = device->storage; storage != 0; storage = storage->next) {
+    freeSpace = ADD_AND_CLAMP(freeSpace, storage->FreeSpaceInBytes, G_MAXUINT64);
+    maxSpace = ADD_AND_CLAMP(maxSpace, storage->MaxCapacity, G_MAXUINT64);

Please add space before opening parenthesis, but again I would not really expect that mtp would return such values if not error...

::: daemon/gvfsbackendnfs.c
@@ +1581,3 @@
         {
+          guint64 avail = MULTIPLY_AND_CLAMP (stat->f_frsize, stat->f_bavail, G_MAXUINT64);
+          guint64 total = MULTIPLY_AND_CLAMP (stat->f_frsize, stat->f_blocks, G_MAXUINT64);

This uses traditional statvfs and also GLib do not special care those values for local filesystems, so not sure again it makes sense. Yes, sanity, but GVfs just unifies different APIs for various libraries/protocol and we can't check everything, we are again just a thin layer between client and libraries...

::: daemon/gvfsbackendsftp.c
@@ +4580,3 @@
     {
+      guint64 avail = MULTIPLY_AND_CLAMP (frsize, bavail, G_MAXUINT64);
+      guint64 total = MULTIPLY_AND_CLAMP (frsize, blocks, G_MAXUINT64);

dtto

::: daemon/gvfsbackendsmb.c
@@ +1599,3 @@
+              block_size = MULTIPLY_AND_CLAMP (st.f_bsize, (st.f_frsize == 0) ? 1 : st.f_frsize, G_MAXUINT64);
+              size = MULTIPLY_AND_CLAMP (st.f_blocks, block_size, G_MAXUINT64);
+              free_space = MULTIPLY_AND_CLAMP (st.f_bfree, block_size, G_MAXUINT64);

dtto

Created attachment 358003 [details] [review]
v2 (simplified)

Alright. Here's a much-simplified version with just the fuse and dav fixes, no macro.

I get that 16 exabytes is probably too big for most backends to worry about. And I get that we're simply unifying and exposing underlying query responses.  But I would still argue that a unifying layer has a responsibility when converting disparate backend data types into its own to make sure the conversion itself isn't making things worse.

But, the dav backend fix is the only pressing one (the only backend I have evidence for failing in the wild).  I'm fine with just a patch for that one.

I would like to see this as two separate patches, but let's push it as is before the release...

Thanks for your contribution!