Bug 1674392

I've repeated the test case on our other machines with RHEL 7.6 and basically the same outcome

[root@z041 ~]# uname -a
Linux z041.pawsey.org.au 3.10.0-957.5.1.el7.x86_64 #1 SMP Wed Dec 19 10:46:58 EST 2018 x86_64 x86_64 x86_64 GNU/Linux
[root@z041 ~]# cat /etc/redhat-release 
Red Hat Enterprise Linux Server release 7.6 (Maipo)

root@z041 ~]# yum search guestfs
Loaded plugins: product-id, search-disabled-repos, subscription-manager
====================================================================================== N/S matched: guestfs ======================================================================================
libguestfs-java.x86_64 : Java bindings for libguestfs
libguestfs-xfs.x86_64 : XFS support for libguestfs
perl-Sys-Guestfs.x86_64 : Perl bindings for libguestfs (Sys::Guestfs)
python-libguestfs.x86_64 : Python bindings for libguestfs
libguestfs.x86_64 : Access and modify virtual machine disk images
libguestfs-inspect-icons.noarch : Additional dependencies for inspecting guest icons
libguestfs-tools.noarch : System administration tools for virtual machines
libguestfs-tools-c.x86_64 : System administration tools for virtual machines
libguestfs-winsupport.x86_64 : Add support for Windows guests to virt-v2v and virt-p2v

[root@z041 ~]# yum install -y libguestfs
[root@z041 ~]# yum install -y libguestfs-tools

Get version

[root@z041 ~]# which guestmount --version
GNU which v2.20, Copyright (C) 1999 - 2008 Carlo Wood.
GNU which comes with ABSOLUTELY NO WARRANTY;
This program is free software; your freedom to use, change
and distribute this program is protected by the GPL.

[root@z041 ~]# rpm -qa |grep -i guestfs
libguestfs-1.38.2-12.el7_6.1.x86_64
libguestfs-tools-c-1.38.2-12.el7_6.1.x86_64
libguestfs-tools-1.38.2-12.el7_6.1.noarch
perl-Sys-Guestfs-1.38.2-12.el7_6.1.x86_64

Steps to Reproduce:
1. Used the OS version of guestfs / libguestfs ie 1.38.2
2. Create a Virtual Volume with enough inodes to create a lot of files Create Virtual file 

[root@z041 ashley]# dd if=/dev/zero of=10^C
[root@z041 ashley]# dd if=/dev/zero of=10gbfile bs=10M count=1024
1024+0 records in
1024+0 records out
10737418240 bytes (11 GB) copied, 4.95509 s, 2.2 GB/s
[root@z041 ashley]# du -sh 10gbfile 
10G	10gbfile
[root@z041 ashley]# /usr/sbin/mkfs.ext4 -i 1024 10gbfile
mke2fs 1.42.9 (28-Dec-2013)
10gbfile is not a block special device.
Proceed anyway? (y,n) y
Discarding device blocks: 2101248/2621440
*snip*

[root@z041 ashley]# mkdir loop
[root@z041 ashley]# mount -t ext4 10gbfile loop/
[root@z041 ashley]# mount |grep -i loop
/tmp/ashley/10gbfile on /tmp/ashley/loop type ext4 (rw,relatime,data=ordered)

3. Populate the Virtual Filesystem with a 1Million+ files in a directory
z041:/tmp/ashley/loop # mkdir 1millfiles
z041:/tmp/ashley/loop # cd 1millfiles/

Create 1.2 Millions files

z042:/tmp/ashley/loop/1millfiles # for i in {1..1200000} ;do touch $i; done

[root@z041 1millfiles]# ls | wc -l
1200000
[root@z041 1millfiles]# ls | sort | tail
999990
999991
999992
999993
999994
999995
999996
999997
999998
999999

umount /tmp/ashley/loop

4. Non Expected results

Do the normal probe of virtual files

Need to start libvirtd as yum install did not start it

[root@z041 ashley]# systemctl start libvirtd

[root@z041 ashley]# guestfish -a /tmp/ashley/10gbfile

Welcome to guestfish, the guest filesystem shell for
editing virtual machine filesystems and disk images.

Type: ‘help’ for help on commands
      ‘man’ to read the manual
      ‘quit’ to quit the shell

><fs> run
 100% ⟦▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒⟧ 00:00
><fs> list-filesystems
/dev/sda: ext4
><fs> exit



Do the guessmount of the virtual filesystem

[root@z041 ashley]# mkdir /tmp/ashley/guestfs
[root@z041 ashley]# guestmount -a /tmp/ashley/10gbfile -m /dev/sda --rw /tmp/ashley/guestfs
[root@z041 ashley]# mount |grep -i fuse
/dev/fuse on /tmp/ashley/guestfs type fuse (rw,nosuid,nodev,relatime,user_id=0,group_id=0)
[root@z041 ashley]# du -sh guestfs/1millfiles/
28M	guestfs/1millfiles/
[root@z041 ashley]# df -i /tmp/ashley/guestfs/
Filesystem       Inodes   IUsed   IFree IUse% Mounted on
/dev/fuse      10485760 1200012 9285748   12% /tmp/ashley/guestfs

You can't do the ls or dir listing of files

root@z041 ashley]# ls
10gbfile  guestfs  loop
[root@z041 ashley]# cd guestfs/
[root@z041 guestfs]# cd 1millfiles/
[root@z041 1millfiles]# ls | sort | tail
ls: reading directory .: Invalid argument

Now If I do the tracing version of guessmount

[root@z041 ashley]# umount guestfs
[root@z041 ashley]# guestmount --trace -a /tmp/ashley/10gbfile -m /dev/sda --rw /tmp/ashley/guestfs

As soon as I do the "ls" in the /tmp/ashley/guestfs/1millfiles

[root@z041 guestfs]# ls
1millfiles  lost+found
[root@z041 guestfs]# cd 1millfiles/
[root@z041 1millfiles]# ls
ls: reading directory .: Invalid argument
[root@z041 1millfiles]# 

The Trace Windows shows there is NULL value being returned when doing the ls in the directory.

libguestfs: trace: readdir "/1millfiles"
libguestfs: trace: readdir = NULL (error)

5. Expected results:

The Virtual Filesystem should be intact, so I'll just remount it

[root@z041 ashley]# mount -t ext4 10gbfile loop/
[root@z041 ashley]# cd loop
[root@z041 loop]# cd 1millfiles/
[root@z041 1millfiles]# ls | tail -n 4
999996
999997
999998
999999

[root@z041 1millfiles]# ls | wc -l
1200000

So it works via the loopback device so the filesystem is intact

(So I've verified with RHEL 7.6 with the latest update)

Cheers then, Ashley

Simple reproducer (without root):

$ rm disk.img
$ truncate -s 10G disk.img
$ mkfs.ext4 -i 1024 disk.img
$ guestfish add disk.img cachemode:unsafe : memsize 4096 : run : mount /dev/sda / : mkdir /files : debug sh 'for i in {1..1200000}; do touch /sysroot/files/$i; done'

$ mkdir /tmp/mnt
$ guestmount -a disk.img -m /dev/sda /tmp/mnt
$ ls /tmp/mnt/files
ls: reading directory '/tmp/mnt/files': Invalid argument

$ fusermount -u /tmp/mnt

As you can see I do get an error (although of course it's still a bug - it ought
to list the files not give an error!)

Hi Richard

Yeah that's pretty much the case, you get the same error ie "ls: reading directory 'XXXXXXXX': Invalid argument"

If you do the guestmount with "--trace" like I did in the ticket what you find is you get a "NULL (error)"

I thought it may be hard coded value but it's more than that.

Cheers then, Ashley

The bug is the same as this TODO item:

https://github.com/libguestfs/libguestfs/blob/89b5dabf8d1797e3875d949b6e2a903a5703be5c/TODO#L498

Long-standing bug which needs to be fixed ...

Hi rjones, do you plan to fix this bug in RHEL-8.6?

I've reproduced this (not with FUSE / guestmount, but with guestfish). I'm getting a specific error from "ll":

> libguestfs: error: ll: guestfsd: failed to encode reply body
> (maybe the reply exceeds the maximum message size in the protocol?)

Which comes from bug 509597, and commits
- 51d0620ad90f ("Don't die if reply message is oversized (RHBZ#509597).", 2009-07-05)
- f826ce487613 ("Fix the error message when reply body is too large (RHBZ#509597).", 2010-03-27)

Re: comment 4, this issue is indeed unfixable in a one-shot-response style protocol, given that we have a limit on the max message in the protocol. No matter how high we set that limit, a longer directory listing can always be generated (in bug 509597, the reproducer was just "find /").

For this, a kind of streaming is required, which I think should already exist somewhere in libguestfs, as we do offer tar-in, tar-out, copy-in, copy-out, which can deal with arbitrarily large files.

This sort of problem was fixed in other APIs, examples include:

1b4d8e2cca7819b4c6e14bb052f40a9fc40205d3
a69f44f56f20beab7793be428df22104f79500e7

dc66dd32c214db99467341caea0be517d2dd736f

8ee51907685d6db92031e84d249ea3bb4cdc2d67

118932fbeac606919f8b96c81184f4f3e0910508

735ce57cda24e2e2f41418d9a9ad34944fefdfb9

(etc, all fixed around the same time 10 years ago)

I believe only readdir was not fixed.  Note that when fixing you can't
change the existing public-visible API.  However adding new APIs
(especially internal_* ones which don't come with any API/ABI guarantee)
is fine, and then use those to reimplement the public API.

> libguestfs: error: ll: guestfsd: failed to encode reply body

Should note this is a different bug from the reported bug.  We could
fix guestfs_ll in the same way that guestfs_ls was fixed, ie.
add new API guestfs_ll0 which streams the data out, reimplement
guestfs_ll to use guestfs_ll0 internally.  See:

https://github.com/libguestfs/libguestfs/blob/0956e8e0c5b70e79a01f43046ac7be2b9a171ed8/lib/file.c#L506-L599

However this bug is about guestfs_readdir which is what guestmount
uses to list large directories.  readdir is much more important because
it's used by other programs.  (No one uses guestfs_ll from a program
because it prints a block of text, literally just the output of "ls -l").

... strange, the "ll" command should already rely on guestfs_ls0(), which is not supposed to have this kind of problem?...

Oh no, "ll" is backed by do_ll() [daemon/ls.c], and that just runs "ls -la" in the daemon :/

OK, understood, thanks.

For streaming a temp file out of the appliance such that it appears (ultimately) in the library as a "guestfs_int_dirent_list" -- are we supposed to come up with our own "bespoke" binary encoding in the temp file, or should we rely on XDR?

Ah, how's about this:

- in do_readdir(), construct the exact same guestfs_int_dirent_list object as before

- afterwards, in do_readdir() [daemon/readdir.c], create an XDR stream manually, with xdrstdio_create(), rather than the xdrmem_create() that reply() uses in "daemon/proto.c". Point this to the temp file.

- rely on the existent xdr_guestfs_int_dirent_list xdr proc for populating the file

- stream the file

- in the guestfs_impl_readdir() function, stream the whole file into memory, and decode it with XDR

Now, what I don't understand is what differentiates *in the generator*, on the library side, a guestfs API implementation that is a simple request-response (such as guestfs_readdir() is right now, sending GUESTFS_PROC_READDIR), from a manually provided implementation (such as guestfs_ls(), which calls guestfs_impl_ls())?

How do I change the "readdir" API's specification in "generator/actions_core.ml" so that the generator call a new function guestfs_impl_readdir() rather than send GUESTFS_PROC_READDIR?

Huh, I guess I need to move "readdir" from "daemon_functions" to "non_daemon_functions" in "generator/actions_core.ml"!

(In reply to Laszlo Ersek from comment #19)
> OK, understood, thanks.
> 
> For streaming a temp file out of the appliance such that it appears
> (ultimately) in the library as a "guestfs_int_dirent_list" -- are we
> supposed to come up with our own "bespoke" binary encoding in the temp file,
> or should we rely on XDR?

There's not a very good answer to this.  Firstly be aware if you're not
already that there is a streaming mechanism already, ie. FileOut).

There are other APIs that need to stream out something which isn't
just text or an existing binary format like 'tar', notably
do_internal_journal_get & tsk (send_dirent_info).  They both use a
big-endian binary protocol, one which is semi-custom, and the other
which is XDR.

Since the rest of the daemon protocol is XDR (and therefore host/endian
independent), it might be a good idea to use XDR there.

Originally we believed that we could handle different guest architectures
by running the daemon under a different qemu emulator, and so the protocol
uses XDR to be host architecture independent, but we never actually
used this.

(In reply to Laszlo Ersek from comment #21)
> Huh, I guess I need to move "readdir" from "daemon_functions" to
> "non_daemon_functions" in "generator/actions_core.ml"!

Yes, if you do this then you can implement your own guestfs_impl_readdir
using internal / lower-level daemon APIs.  See also guestfs_impl_ls.

(In reply to Laszlo Ersek from comment #20)
> Ah, how's about this:
> 
> - in do_readdir(), construct the exact same guestfs_int_dirent_list object
> as before

It'll be something like do_internal_readdir, with a FileOut parameter, but yes.

> - afterwards, in do_readdir() [daemon/readdir.c], create an XDR stream
> manually, with xdrstdio_create(), rather than the xdrmem_create() that
> reply() uses in "daemon/proto.c". Point this to the temp file.

You'll actually have to use xdrmem_create + send_file_write + send_file_end.

> - rely on the existent xdr_guestfs_int_dirent_list xdr proc for populating
> the file

Possibly yes, I can't remember the details here.

> - stream the file
> 
> - in the guestfs_impl_readdir() function, stream the whole file into memory,
> and decode it with XDR

Probably stream it to a temporary file (see guestfs_impl_ls) and
decode it from there.

> Now, what I don't understand is what differentiates *in the generator*, on
> the library side, a guestfs API implementation that is a simple
> request-response (such as guestfs_readdir() is right now, sending
> GUESTFS_PROC_READDIR), from a manually provided implementation (such as
> guestfs_ls(), which calls guestfs_impl_ls())?
> 
> How do I change the "readdir" API's specification in
> "generator/actions_core.ml" so that the generator call a new function
> guestfs_impl_readdir() rather than send GUESTFS_PROC_READDIR?

There are functions which are implemented in the daemon ("daemon functions"),
where all you have to do is wrote a do_function inside the daemon.  And
there are function which are implemented in the library ("non-daemon functions",
should probably be called "library functions"), where all you have to do
is wrote guestfs_impl_function.  The generator provides all the rest of
the infrastructure automatically.

Non-daemon functions don't have a proc number because the proc number is
used by the daemon to dispatch calls.

Funnily enough, I had worked on the yara stuff before, and yara uses the exact same pattern. Another example is commit 19e7a52f8f1a ("New API: filesystem_walk", 2016-06-22).

lib/tsk.c:  xdrstdio_create (&xdr, fp, XDR_DECODE);
lib/yara.c:  xdrstdio_create (&xdr, fp, XDR_DECODE);

So it seems like for any such function (with internal streaming), we need an "internal_STUFF" daemon function taking a FileOut, and also getting a proc number (due to it being a daemon function), and a "STUFF" non-daemon function, which creates a temp file, manually calls the former function, and then de-serializes the temp file.

Yes, although TBH there aren't many such cases and we've also fixed
most of those in different ad hoc ways.  Also some calls (like guestfs_ls)
have daemon implementations (guestfs_ls0) which are not internal
because they're useful and supportable.

This can be reproduced with fewer files. We encode/decode the following structure with XDR:

struct guestfs_int_dirent {
	int64_t ino;
	char ftyp;
	char *name;
};

The "name" field is populated from "d_name" (256 bytes incl. the terminating NUL). So for the XDR encoding, if we use filenames consisting of 255 (non-NUL) bytes, we minimally need 8 + 1 + 255 = 264 bytes just for the raw data (i.e., XDR metadata not counted), per file.

GUESTFS_MESSAGE_MAX is 4MB, thus the symptom can be triggered with as few as 15888 files; which requires approx. two orders of magnitude fewer inodes than what's described in the original report, and the default "guestfish -N" image size of 1GB suffices:

rm -f test1.img
guestfish -N fs:ext4 quit
losetup /dev/loop0 test1.img
kpartx -v -a /dev/loop0
if ! [ -b /dev/loop0p1 ]; then
  mknod /dev/loop0p1 b 253 1
fi
mount /dev/loop0p1 /mnt
(
  cd /mnt
  mkdir x
  cd x
  Y=$(yes | head -n 249 | tr -d '\n')
  seq -f "$Y-%05.0f" 0 15887 \
  | xargs -r -- touch --
)
umount /mnt
kpartx -d /dev/loop0
losetup -d /dev/loop0
losetup

(The script uses losetup etc because the image is created afresh, so it can be trusted, *and* this way we can use touch with xargs -- much faster.)

guestmount -a test1.img -m /dev/sda1 /mnt

ls -l /mnt
> total 6064
> drwx------. 2 root root   16384 Apr 30 06:29 lost+found
> drwxr-xr-x. 2 root root 6193152 Apr 30 06:29 x

ls -l /mnt/x
> ls: reading directory '/mnt/x': Invalid argument
> total 0

I've got a functional patch, but I see an opportunity to improve its performance (in a second patch).

[libguestfs PATCH 0/2] lift protocol limit on guestfs_readdir()
Message-Id: <20220501070442.6231-1-lersek>
https://listman.redhat.com/archives/libguestfs/2022-May/028770.html

[libguestfs PATCH v2 0/2] lift protocol limit on guestfs_readdir()
Message-Id: <20220502085601.15012-1-lersek>
https://listman.redhat.com/archives/libguestfs/2022-May/028781.html

(In reply to Laszlo Ersek from comment #30)
> [libguestfs PATCH v2 0/2] lift protocol limit on guestfs_readdir()
> Message-Id: <20220502085601.15012-1-lersek>
> https://listman.redhat.com/archives/libguestfs/2022-May/028781.html

Upstream commit range ac00e603f838..4864d21cb8eb.

Tested with libguestfs-1.48.2-2.el9.x86_64 on RHEL9.1 host:


Steps:

$ truncate -s 10G disk.img
$ mkfs.ext4 -i 1024 disk.img
$ guestfish add disk.img cachemode:unsafe : memsize 4096 : run : mount /dev/sda / : mkdir /files : debug sh 'for i in {1..1200000}; do touch /sysroot/files/$i; done'
$ mkdir /tmp/mnt
$ guestmount -a disk.img -m /dev/sda /tmp/mnt
$ ls /tmp/mnt/files
...
1067495  1134995  122494   189996  257495  324995  392495  459996  527495  594996  662495  729996  797496  864996  932495  999997
1067496  1134996  122495   189997  257496  324996  392496  459997  527496  594997  662496  729997  797497  864997  932496  999998
1067497  1134997  122496   189998  257497  324997  392497  459998  527497  594998  662497  729998  797498  864998  932497  999999

All the files are listed.

Verified this bug since the test case for this bug has been automated and passed in the latest nightly compose test.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Low: libguestfs security, bug fix, and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2022:7958