Bug 61472 - various patches: don't use access(), error handling, use fdatasync()
Summary: various patches: don't use access(), error handling, use fdatasync()
Status: RESOLVED FIXED
Alias: None
Product: shared-mime-info
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Shared Mime Info group
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-26 00:38 UTC by Colin Walters
Modified: 2013-12-09 01:45 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
0001-Don-t-use-access-to-check-for-writability.patch (1.17 KB, patch)
2013-02-26 00:38 UTC, Colin Walters
Details | Splinter Review
0002-Actually-check-for-errors-when-closing-streams-etc.patch (9.81 KB, patch)
2013-02-26 00:39 UTC, Colin Walters
Details | Splinter Review
0003-Call-fdatasync-before-renaming-files-into-place.patch (1.92 KB, patch)
2013-02-26 00:39 UTC, Colin Walters
Details | Splinter Review
updated for comments (11.03 KB, patch)
2013-02-26 17:44 UTC, Colin Walters
Details | Splinter Review
call fdatasync, just rebased for ordering (1.92 KB, patch)
2013-02-26 17:44 UTC, Colin Walters
Details | Splinter Review
don't use access, updated for comments (1.18 KB, patch)
2013-02-26 17:45 UTC, Colin Walters
Details | Splinter Review

Description Colin Walters 2013-02-26 00:38:52 UTC
Created attachment 75536 [details] [review]
0001-Don-t-use-access-to-check-for-writability.patch

These are to make shared-mime-info better for OSTree.
Comment 1 Colin Walters 2013-02-26 00:39:21 UTC
Created attachment 75537 [details] [review]
0002-Actually-check-for-errors-when-closing-streams-etc.patch
Comment 2 Colin Walters 2013-02-26 00:39:35 UTC
Created attachment 75538 [details] [review]
0003-Call-fdatasync-before-renaming-files-into-place.patch
Comment 3 Bastien Nocera 2013-02-26 10:23:33 UTC
Comment on attachment 75536 [details] [review]
0001-Don-t-use-access-to-check-for-writability.patch

Review of attachment 75536 [details] [review]:
-----------------------------------------------------------------

I'd like to see a similar error message as the one you're removing when writing fails, by checking the errno.
Comment 4 Bastien Nocera 2013-02-26 10:27:57 UTC
Comment on attachment 75537 [details] [review]
0002-Actually-check-for-errors-when-closing-streams-etc.patch

Review of attachment 75537 [details] [review]:
-----------------------------------------------------------------

::: update-mime-database.c
@@ +980,4 @@
>  
> +	if (!atomic_update(filename, &local_error))
> +	{
> +		g_printerr("%s\n", local_error->message);

Free the error here too.

@@ +3457,4 @@
>  {
>  	FILE *stream = fopen(filename, "wb");
>  
> +

Some extra line feeds here.

@@ +3811,5 @@
>  
> +out:
> +	if (local_error != NULL)
> +	{
> +		g_printerr("%s\n", local_error->message);

Please free the error, even if it doesn't really matter.
Comment 5 Bastien Nocera 2013-02-26 10:35:20 UTC
Comment on attachment 75538 [details] [review]
0003-Call-fdatasync-before-renaming-files-into-place.patch

Review of attachment 75538 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 6 Colin Walters 2013-02-26 17:44:17 UTC
Created attachment 75586 [details] [review]
updated for comments
Comment 7 Colin Walters 2013-02-26 17:44:42 UTC
Created attachment 75587 [details] [review]
call fdatasync, just rebased for ordering
Comment 8 Colin Walters 2013-02-26 17:45:30 UTC
Created attachment 75588 [details] [review]
don't use access, updated for comments

Note with this one the "run me as root" hint is already in the first patch.
Comment 9 Bastien Nocera 2013-02-27 10:18:00 UTC
Pushed, thanks for the patches.
Comment 10 Richard W.M. Jones 2013-04-02 17:47:01 UTC
comment 0 patch:
> Don't use access() to check for writability
> 
> The gnome-ostree build system uses a FUSE mount to access disk images
> as non-root.  The files appear to be owned by root, but are actually
> modifable by the build user.  Unfortunately, the access() system call
> is unaware of this, and will error out.

I'll just tell you a little bit about why this happens.

In guestmount (the FUSE process), when asked to get the stat of a file
we call this getattr function:

https://github.com/libguestfs/libguestfs/blob/d0a8cca9a1a399d43c6e44dfc655eedbe61d22ec/src/fuse.c#L240

which for a Unix filesystem just returns the precise same fields that
the 'stat(2)' syscall returned.  In particular this doesn't do any
local username translation -- it's literally just UIDs passed through
from the filesystem being examined up through to the host's userspace
(in this case st_uid == 0 == root, but in other cases you could get
wrong usernames).

Conversely when modifying the filesystem, because the libguestfs daemon
runs as "root" inside its qemu appliance, there is no checking at all.
Anyone who can get write access to the mountpoint on the host will
effectively have "root" access to the target filesystem.

*Who* can get access is rather complex and depends on some details in
FUSE and also what FUSE mount options are used.  Some options which
affect this are '-o allow_other', '-o uid=...' and '-o gid=...' and
there may be others.  Also if the mountpoint is in a private namespace,
and/or if the mountpoint is located inside a directory which is not
accessible.

guestmount takes no special action to prevent writes from users who
managed to "get to" the mountpoint.

Your problem is interesting because the access(2) call (in the host
kernel) literally doesn't have the information it needs to determine
the true writability of the file.  I would argue that access(W_OK)
never really has this information since even for a regular non-FUSE
filesystem there are lots of things that can get in the way of writes
(eg. a readonly block device or readonly NFS server).

It's probably best (as you have done) to just ignore pre-tests and go
ahead and try the write.  If it succeeds, all good.  If it fails, you'll
get a true indication why.
Comment 11 Fryderyk Dziarmagowski 2013-10-28 08:09:36 UTC
fdatasync fix is causing a massive slowdown when updating mime cache:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 30.57    0.029584          42       705           rename
 28.50    0.027582          39       705           fdatasync
 15.24    0.014751          10      1431           open
  8.68    0.008400          11       791           write
  5.08    0.004919           3      1443           close
  4.27    0.004131           6       706           munmap
  2.84    0.002749           4       737           mmap
  1.94    0.001873           3       693       693 mkdir
  1.18    0.001141           2       717           fstat
  0.71    0.000686           2       371           brk
  0.55    0.000537           4       144           read
  0.22    0.000213           9        24           getdents
  0.07    0.000070           4        16           mprotect
  0.07    0.000065           2        33           stat
  0.03    0.000029           2        13         1 openat
  0.01    0.000014           1        11           lseek
  0.01    0.000013           4         3         1 access
  0.00    0.000004           2         2           rt_sigaction
  0.00    0.000003           3         1           execve
  0.00    0.000002           2         1           rt_sigprocmask
  0.00    0.000002           2         1           getrlimit
  0.00    0.000002           2         1           futex
  0.00    0.000002           2         1           set_tid_address
  0.00    0.000002           2         1           set_robust_list
  0.00    0.000001           1         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00    0.096775                  8552       695 total
strace -c update-mime-database -V /usr/share/mime  0.30s user 0.19s system 2% cpu 22.229 total

vs% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 47.36    0.014699          21       705           rename
 17.98    0.005582           8       726           open
 11.89    0.003692           5       791           write
  5.62    0.001743           2       706           munmap
  3.80    0.001179           2       737           mmap
  3.30    0.001023           1       693       693 mkdir
  3.14    0.000974           1       738           close
  2.10    0.000653           1       717           fstat
  1.96    0.000607           2       371           brk
  1.56    0.000484           3       144           read
  0.70    0.000217           9        24           getdents
  0.20    0.000063           4        16           mprotect
  0.19    0.000060           2        33           stat
  0.09    0.000027           2        13         1 openat
  0.04    0.000012           1        11           lseek
  0.04    0.000011           4         3         1 access
  0.01    0.000003           2         2           rt_sigaction
  0.01    0.000002           2         1           rt_sigprocmask
  0.01    0.000002           2         1           execve
  0.01    0.000002           2         1           getrlimit
  0.01    0.000002           2         1           futex
  0.00    0.000001           1         1           arch_prctl
  0.00    0.000001           1         1           set_tid_address
  0.00    0.000001           1         1           set_robust_list
------ ----------- ----------- --------- --------- ----------------
100.00    0.031040                  6437       695 total
strace -c update-mime-database -V /usr/share/mime  0.24s user 0.08s system 98% cpu 0.319 total

30s vs <1s


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.