Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt
Hi,
i wrote:
> > Assuming variable "xfail" is be set to a non-empty string exactly if
> > argument "--xfail" is given, i'd replace:
> >
> > if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
> > rm -rf "$lukstestdir" || :
> > fi
Glenn Washburn wrote:
> RET should never be undefined because $? always has a numerical value.
> And in my change I default xfail to 0. So I change the if statement to:
>
> if [ -z "$debug" ] && [ "$RET" -eq "$xfail" ]; then
>
> This is a lot simpler, though perhaps a tad less clear. Do you see a
> reason why this is less desirable?
If indeed in a successful --xfail test RET must have the value of "$xfail"
(i guess 1), then it is concise and sufficient.
If other non-zero values of RET are acceptable as indication of an
intended failure then your proposal is too sparse.
In this case i would propose to flatly enumerate the two cases which shall
lead to removal of the test data:
if [ -z "$debug" ] && [ "$xfail" -eq 1 ] && [ "$RET" -ne 0 ]; then
rm -rf "$lukstestdir" || :
elif [ -z "$debug" ] && [ "$xfail" -eq 0 ] && [ "$RET" -eq 0 ]; then
rm -rf "$lukstestdir" || :
fi
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt
Hi,
Glenn Washburn wrote:
> [...] grub-shell-luks-tester cleans up after
> itself, if it returns success. grub_cmd_cryptomount has a test that
> expects failure. But grub-shell-luks-tester doesn't know that this is
> an expected failure and should cleanup and grub_cmd_cryptomount doesn't
> ever cleanup after grub-shell-luks-tester. Perhaps
> grub-shell-luks-tester should be passed a parameter to indicate
> expected failure (eg. --xfail). Have any other ideas?
I agree to the idea of an option to invert the effect of
[ "$RET:-1" -eq 0 ] in cleanup() of tests/util/grub-shell-luks-tester.in .
Assuming variable "xfail" is be set to a non-empty string exactly if
argument "--xfail" is given, i'd replace:
if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
rm -rf "$lukstestdir" || :
fi
by:
if [ -z "$debug" ]; then
if [ -n "$xfail" ]; then
if [ "${RET:-0}" -ne 0 ]; then
rm -rf "$lukstestdir" || :
fi
else
if [ "${RET:-1}" -eq 0 ]; then
rm -rf "$lukstestdir" || :
fi
fi
fi
Equivalent, but heavily economizing on line count would be:
if [ -z "$debug" ] && [ -n "$xfail" ] && [ "${RET:-0}" -ne 0 ]; then
rm -rf "$lukstestdir" || :
elif [ -z "$debug" ] && [ -z "$xfail" ] && [ "${RET:-1}" -eq 0 ]; then
rm -rf "$lukstestdir" || :
fi
I tested both code pieces in a dry-run script with the 12 variations of
debug={"", "1"} , xfail={"", "1"} , RET={undefined, "0", "1"} .
Decision for removal happened only with:
(debug="", xfail="", RET="0")
(debug="", xfail="1", RET="1")
The first code performs less []-expressions and seems clearer to me at
the price of doubled line count.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt
Hi, i wrote: > > I meanwhile forgot why i proposed > > > +unset TMPDIR > > but there was some theoretical reason for this ... Glenn Washburn wrote: > My guess was that you wanted to unexport TMPDIR. Yes. The reason was that i wanted to limit the effect of exporting to the run of grub-shell-luks-tester, just in case that any further code does not expect exported TMPDIR. > > LUKS2 test with argon2 pbkdf: XFAIL > XFAIL means "expected fail", so its okay. But why doesn't it clean up then ? This particular sub-test leaves not only the empty ./mnt firectory but also a 20 MB device image (?) and some small data files. All other sub-tests of tests/grub_cmd_cryptomount leave a neatly empty directory which grub_cmd_cryptomount can rmdir after the sub-test is done. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] tests/util/grub-shell-luks-tester: Add missing line to create RET variable in cleanup
Hi, please ignore my proposal about "set -e". My assessment of "set -e" in tests/util/grub-shell-luks-tester.in is obviously wrong, probably because "set -e" is not inherited by sub-shells. The line #! @BUILD_SHEBANG@ -e enables script abort on command error, but causes reply errexit on only with echo set -o | grep errexit but not with echo "grub-shell-luks-tester.in: $(set -o | grep errexit)" >>/tmp/grub-shell-luks-tester.errexit.log Sorry for the noise. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] tests/util/grub-shell-luks-tester: Add missing line to create RET variable in cleanup
Hi,
(adding Vladimir Serbinenko to Cc)
Glenn Washburn wrote:
> --- a/tests/util/grub-shell-luks-tester.in
> +++ b/tests/util/grub-shell-luks-tester.in
> @@ -143,6 +143,7 @@ fi
>
> # Make sure that the dm-crypto device is shutdown
> cleanup() {
> +RET=$?
> if [ -e "$luksdev" ]; then
> cryptsetup close "$luksdev"
> fi
The change itself looks good to me.
But i think that there is
set -e
missing in
tests/util/grub-shell-luks-tester.in
and possibly in
tests/grub_cmd_cryptomount.in
At least inserted commands:
echo "grub_cmd_cryptomount.in: $(set -o | grep errexit)"
echo "grub-shell-luks-tester.in: $(set -o | grep errexit)"
>>/tmp/grub-shell-luks-tester.errexit.log
say in grub_cmd_cryptomount.log and grub-shell-luks-tester.errexit.log:
grub_cmd_cryptomount: errexit off
grub-shell-luks-tester.in: errexit off
The neighbors of grub-shell-luks-tester.in have such a "set -e":
tests/util/grub-fs-tester.in
tests/util/grub-shell-tester.in
tests/util/grub-shell.in
Many tests/*.in have it too. So i propose to add it to
tests/grub_cmd_cryptomount.in
as well.
I came to this while trying whether it is it guaranteed that "$?" is
always meaningful, given the fact that cleanup() can be called from
various places in the script by
trap cleanup EXIT INT TERM KILL QUIT
My experiment was to press Ctrl+C while this shell code was sleeping
cleanup() {
RET=$?
echo "RET=$RET"
}
trap cleanup EXIT INT TERM KILL QUIT
sleep 10
ls /notexisting
I got two output lines:
RET=130
RET=2
So without "set -e" cleanup() can be called more than once.
I think that grub-shell-luks-tester.in does not expects this.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt
Hi,
The patches apply without complaints by "git am".
But when i run (again as superuser, shudder):
make check TESTS=grub_cmd_cryptomount
i still get in /tmp the empty direcories /tmp/17*.LUKS2_*.
Minor nitpick:
> [PATCH 2/4] tests: Cleaup the cryptsetup script in grub_cmd_cryptomount
> unless debug is enabled
Typo in subject: "Cleaup".
I think the reason for the remaining empty directories is that i do not
find in the patches any equivalent of this proposal of mine:
--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -46,10 +48,22 @@ _testcase() {
...
> mkdir -p "$TMPDIR"
>
> output=`"$@" 2>&1` || res=$?
> +
> +if [ -z "$debug" ]; then
> +if ! rmdir "$TMPDIR" >/dev/null 2>&1; then
> +echo
> +echo "Note: Temporary directory cannot be removed:"
> +echo "$TMPDIR"
> +echo " Please inspect and remove manually."
> +echo
> +fi
> +fi
> +unset TMPDIR
> TMPDIR=$_TMPDIR
>
> if [ "$res" -eq "$EXPECTEDRES" ]; then
Essential would be some rmdir to remove the directory created by
mkdir -p "$TMPDIR"
If i add above proposed code to tests/grub_cmd_cryptomount.in then no
empty directories of form /tmp/17*.LUKS2_* remain.
See below for non-empty /tmp/1727105940.LUKS2_test_with_argon2_pbkdf .
I meanwhile forgot why i proposed
> +unset TMPDIR
but there was some theoretical reason for this ...
--
Possibly another problem to tackle:
One of the debris directories is not empty and thus would trigger the
"cannot be removed" warning:
/tmp/1727104207.LUKS2_test_with_argon2_pbkdf
contains
grub-shell-luks-tester.Pb6yqhgZSr
The offender is itself a directory with this content:
-rw-r--r-- 1 root root 20971520 Sep 23 17:10 luks.disk
-rw-r--r-- 1 root root8 Sep 23 17:10 luks.key
drwxr-xr-x 2 root root 4096 Sep 23 17:10 mnt
-rw-r--r-- 1 root root 1203 Sep 23 17:10 testcase.cfg
-rw-r--r-- 1 root root 23 Sep 23 17:10 testoutput
-rw-r--r-- 1 root root 243 Sep 23 17:10 testvars
Subdirectory ./mnt is empty.
File
grub_cmd_cryptomount.log
contains among many "PASS" lines:
LUKS2 test with argon2 pbkdf: XFAIL
but nothing more enlightening.
The last line says
PASS grub_cmd_cryptomount (exit status: 0)
---
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Change "efi" to "EFI" in grub-mkrescue for secure boot
Hi, Pascal Hambourg wrote: > When > booting from a EFI partition on a regular disk, then $root is set to the > EFI partition (hdX,Y). When booting from a EFI El Torito image on a CD, > then $root is set to the whole CD (cdX), and the El Torito image is not > visible. That's a surprise to me. I wrote (before knowing this information): > > I can confirm that many distro ISOs have a copy of the file tree of their > > EFI system partition as file tree "/EFI/boot" in the ISO 9660 filesystem. > > This copy normally plays no role in booting Pascal Hambourg wrote: > It plays a role in Debian ISOhybrid images when booting from CD with a > monolithic signed GRUB image which fetches an early grub.cfg in the EFI > partition, because GRUB does not see the El Torito image and can read > the early grub.cfg only from the ISO 9660 filesystem. I was disbelieving enough to make a DVD+RW from debian-12.7.0-amd64-netinst.iso with /EFI/debian/grub.cfg removed from the ISO filesystem: xorriso -indev debian-12.7.0-amd64-netinst.iso \ -outdev /dev/sr3 \ -blank as_needed \ -rm /EFI/debian/grub.cfg -- \ -boot_image any replay \ -assess_indev_features replay Indeed when booting this DVD on real iron, i end up at a "grub>" prompt. The original debian-12.7.0-amd64-netinst.iso put later on the same DVD boots to the installer menu. There remains my curiosity why this difference between booting via El Torito and via partition table from the same FAT filesystem exists and whether this is a bug. (Well, by tradition it probably is a feature now.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Change "efi" to "EFI" in grub-mkrescue for secure boot
Hi,
Askar Safin wrote:
> I CC Thomas Schmitt, because my work seems to be related to xorriso.
I checked that grub-mkrescue variables efidir_efi and efidir_efi_boot
are not leading to arguments of the xorriso run.
Insofar this change is transparent from the view of xorriso.
I can confirm that many distro ISOs have a copy of the file tree of their
EFI system partition as file tree "/EFI/boot" in the ISO 9660 filesystem.
Nobody seems to complain about the mixed case.
This copy normally plays no role in booting but is present for reference
and for copying out by e.g. Rufus. This is convenient because else one
has to search and mount the EFI system partition.
(I cannot judge whether "grub-mkimage -p /EFI/debian" has any relation
to the names in the ISO 9660 filesystem or whether the filesystem where
it matters is case sensitive.)
> iso9660 file system is case sensitive from GRUB point of view.
That's probably because grub-core/fs/iso9660.c has support for Rock Ridge.
One would have to use xorrisofs option --norock to keep xorriso from
producing Rock Ridge. Omitting -R, -rock, -r, or -rational-rock would
not suffice, other than with mkisofs and genisoimage.
> Signed-off-by: Askar Safin
> ---
> util/grub-mkrescue.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
> index 6dc71a8a1..c78647ebe 100644
> --- a/util/grub-mkrescue.c
> +++ b/util/grub-mkrescue.c
> @@ -771,8 +771,12 @@ main (int argc, char *argv[])
>|| source_dirs[GRUB_INSTALL_PLATFORM_RISCV64_EFI])
> {
>FILE *f;
> - char *efidir_efi = grub_util_path_concat (2, iso9660_dir, "efi");
> - char *efidir_efi_boot = grub_util_path_concat (3, iso9660_dir, "efi",
> "boot");
> +
> + /* This directory should be named "EFI", not "efi". See September
> 2024 grub-devel discussion for details
> + ( https://lists.gnu.org/archive/html/grub-devel/2024-09/index.html
> )
> + */
> + char *efidir_efi = grub_util_path_concat (2, iso9660_dir, "EFI");
> + char *efidir_efi_boot = grub_util_path_concat (3, iso9660_dir, "EFI",
> "boot");
> char *imgname, *img32, *img64, *img_mac = NULL;
>char *efiimgfat, *iso_uuid_file, *diskdir, *diskdir_uuid;
>grub_install_mkdir_p (efidir_efi_boot);
>
Reviewed-by: Thomas Schmitt
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
Hi,
i post this as base of discussion, not yet as sincere patch proposal.
It seems desirable to bundle the temporary data of each particular test
in tests/grub_cmd_cryptomount.in in a single directory, as it seems
intended by the code but currently is not achieved.
So i propose to default TMPDIR to "/tmp" and to export it after it
was augmented by a test specific subdirectory component.
In order to let tests/grub_cmd_cryptomount.in finally remove this work
directory, we can either do "rm -rf" or let the subordinate workers clean
up their work directories which they create as subdirectories.
I am in favor of the latter.
This implies a possibly intrusive change in
tests/util/grub-shell-luks-tester.in
where i propose to drop the evaluation of a variable named RET, which i
cannot find to be set or used anywhere in the git repo tree.
Another change would be in
tests/util/grub-shell.in
where i propose to finally remove the work directory.
In order to avoid large colateral damage, i propose rmdir rather than
"rm -rf". (If debris remains in the work directory than this should be
investigated by the user.)
Finally i propose to remove a regular file which the last test of
grub_cmd_cryptomount.in currently leaves in /tmp.
I add a diff to the code which now works for me and leaves no debris
in /tmp after
make check TESTS=grub_cmd_cryptomount
In case somebody finds parts of it useful:
Signed-off-by: Thomas Schmitt
Have a nice day :)
Thomas
diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
index f4d8f3547..5ce97fa06 100644
--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -37,6 +37,8 @@ fi
COMMON_OPTS='${V:+--debug=$V} --cs-opts="--pbkdf-force-iterations 1000"'
+debug=${GRUB_SHELL_DEFAULT_DEBUG:-$GRUB_TEST_DEFAULT_DEBUG}
+
_testcase() {
local EXPECTEDRES=$1
local LOGPREFIX=$2
@@ -46,10 +48,22 @@ _testcase() {
# Create a subdir in TMPDIR for each testcase
_TMPDIR=$TMPDIR
-TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e
's,:$,,g'`
+TMPDIR="${TMPDIR:-/tmp}"/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[
/],_,g' -e 's,:$,,g'`
+export TMPDIR
mkdir -p "$TMPDIR"
output=`"$@" 2>&1` || res=$?
+
+if [ -z "$debug" ]; then
+if ! rmdir "$TMPDIR" >/dev/null 2>&1; then
+echo
+echo "Note: Temporary directory cannot be removed:"
+echo "$TMPDIR"
+echo " Please inspect and remove manually."
+echo
+fi
+fi
+unset TMPDIR
TMPDIR=$_TMPDIR
if [ "$res" -eq "$EXPECTEDRES" ]; then
@@ -182,4 +196,6 @@ eval testcase "'LUKS2 test with second key slot and first
slot using different p
@builddir@/grub-shell-luks-tester $LUKS2_COMMON_OPTS $COMMON_OPTS \
"--cs-script='$csscript'"
+test -n "$debug" || rm "$csscript"
+
exit 0
diff --git a/tests/util/grub-shell-luks-tester.in
b/tests/util/grub-shell-luks-tester.in
index b2a8a91b4..20ebecacf 100644
--- a/tests/util/grub-shell-luks-tester.in
+++ b/tests/util/grub-shell-luks-tester.in
@@ -146,7 +146,7 @@ cleanup() {
if [ -e "$luksdev" ]; then
cryptsetup close "$luksdev"
fi
-if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
+if [ -z "$debug" ] ; then
rm -rf "$lukstestdir" || :
fi
}
diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index ae5f711fe..98f7e7394 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -711,6 +711,7 @@ test -n "$debug" || rm -f "${isofile}"
test -n "$debug" || rm -rf "${rom_directory}"
test -n "$debug" || rm -f "${tmpfile}" "${cfgfile}" "${goutfile}"
test -n "$debug" || rm -f "$work_directory/run.sh"
+test -n "$debug" || rmdir "$work_directory"
exit $ret
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
Hi,
the submission of v2 is hampered by the fact that the artful composition
of TMPDIR in tests/grub_cmd_cryptomount.in does not influence the worker
tests/util/grub-shell-luks-tester.in because TMPDIR is not exported.
So grub-shell-luks-tester creates its workdirectory directly in /tmp
rather than in /tmp/1724008906.LUKS1_test_key_file_with_offset_and_size/ .
More problems show up:
-
grub-shell-luks-tester does _not_ clean up.
That's because in a run of
make check TESTS=grub_cmd_cryptomount
the situation in function cleanup() of grub-shell-luks-tester is:
lukstestdir='/tmp/grub-shell-luks-tester.S2rhmuJJlf'
debug=''
RET=''
{RET:-1}='1'
So the condition in the following if-clause is false because [ 1 -eq 0 ]:
if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
rm -rf "$lukstestdir" || :
fi
and no rm -rf happens.
I riddle from where RET shall get its expected content 0.
-
After each make check round, another single directory is left:
/tmp/tmp.yp1z11pq2w
of the same age as the grub-shell-luks-tester directories.
It stems from this gesture in grub-shell-luks-tester.in:
# Add good password to second slot and change first slot to unchecked password
csscript=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 99
I proved this by changing "tmp.XX" to "tmp_css.XX" and
then getting /tmp/tmp_css.5wRYh4CUaL .
-
Another problem is that the test leaves empty directories of the form
/tmp/grub-shell.2jWNHEhpA2
(which also end up in TMPDIR if it is exported).
-
So i will have to investigate more ... and would not mind if others
would take this issue out of my hands.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
Hi, thinking more i believe that the currently used mkdir option -p is inappropriate in tests/grub_cmd_cryptomount.in . It hampers proper cleanup because the script cannot know how many directories in the path to TMPDIR were created and need to be removed. It is unusual because about all other tests in GRUB assume that non-empty TMPDIR or its default "/tmp" lead to an existing directory. (At least my local mktemp(1) refuses on non-existing components in the path.) If i read no objections, i will propose removal of -p in patch v2. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
Hi,
i wrote:
> > Further delete each created directory as soon as the command of its
> > test case is finished.
> > [...]
> > mkdir -p "$TMPDIR"
> >
> > output=`"$@" 2>&1` || res=$?
> > +
> > +rmdir "$TMPDIR"
Daniel Kiper wrote:
> s/rmdir/rm -rf/?
This is equivalent to the question whether remaining content shall be
removed silently. In my case the directories were all empty.
The worker in TMPDIR is @builddir@/grub-shell-luks-tester which stems
from tests/util/grub-shell-luks-tester.in . It shows own effort to leave
a clean TMPDIR:
cleanup() {
...
if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
rm -rf "$lukstestdir" || :
fi
}
trap cleanup EXIT INT TERM KILL QUIT
...
lukstestdir="`mktemp -d "${TMPDIR:-/tmp}/$(basename "$0").XX"`" ||
exit 20
Seeing the condition before "rm -rf", i guess tests/grub_cmd_cryptomount
should rather not remove the directory if it is not empty. A smarter way
than letting rmdir loudly fail seems appropriate, though.
If you agree to my assessment i will try to propose one in v2.
---
Self criticism:
I have recognized meanwhile that the proposed gesture
> > +: ${TMPDIR:=/tmp}
stems from the gnulib subdirectory and is not tradition in the rest
of the GRUB git repo.
GRUB test tradition seems to be temporary defaulting of TMPDIR like in
line 174 of tests/grub_cmd_cryptomount.in :
csscript=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 99
Indeed the patch would be less needy of a lengthy comment if i propose
something like this yet untested change instead:
-TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e
's,:$,,g'`
+TMPDIR="${TMPDIR:-/tmp}"/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[
/],_,g' -e 's,:$,,g'`
If you agree to my assessment, i will propose this in patch v2.
(Tested, of course.)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: make check as superuser leaves empty directories in /-directory
Hi,
with the freshly submitted patch applied, i see as superuser:
# make check TESTS=grub_cmd_cryptomount
...
PASS: grub_cmd_cryptomount
...
# echo $?
0
#
No new directories appeared in the root directory. During the "make check"
run i could spot single files /tmp/*LUKS* which did not exist any more
soon after.
By help of Greg Wooledge on debian-user mailing i was able to find the
description of ${TMPDIR=/tmp} in man bash and man dash.
"=" sets the variable to /tmp only if TMPDIR is not set.
":=" sets also if it is set to empty text.
The use of TMPDIR in tests/ is mainly made safe by ${TMPDIR:-/tmp}, not
by ${TMPDIR-/tmp}. So i deem ${TMPDIR:=/tmp} the right thing to do,
if not every expansion of $TMPDIR shall be made safe individually.
If the gnulib tests are run by "make check", then one should review
the use of ${TMPDIR=/tmp} in
./gnulib/tests/test-copy-acl.sh
./gnulib/tests/test-file-has-acl.sh
./gnulib/tests/test-set-mode-acl.sh
./gnulib/tests/test-copy-file.sh
./gnulib/tests/test-parse-duration.sh
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
If not TMPDIR is set by the user then the test grub_cmd_cryptomount
creates about 20 directories named *LUKS*_test* in the root directory
and leaves them there when the test ends.
Initialize in the test script the variable TMPDIR to /tmp if it is not
set or if it set to empty text. To be consistent with the usage of
${TMPDIR:-/tmp} in the script, use ${TMPDIR:=/tmp} not ${TMPDIR=/tmp}.
Further delete each created directory as soon as the command of its
test case is finished.
Signed-off-by: Thomas Schmitt
---
tests/grub_cmd_cryptomount.in | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
index f4d8f3547..696e61e96 100644
--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -44,12 +44,23 @@ _testcase() {
local output
shift 2
+# Use the environment variable TMPDIR, falling back to /tmp. This allows
+# users to specify a different temporary directory, for example, if their
+# /tmp is filled up or too small.
+# Some other GRUB tests use this gesture with "=" rather than ":=".
+# But in sync with the many occurences of ${TMPDIR:-/tmp}, this test uses
+# ":=" to fill empty TMPDIR with "/tmp", regardless whether TMPDIR was
+# set to empty or was not set at all.
+: ${TMPDIR:=/tmp}
+
# Create a subdir in TMPDIR for each testcase
_TMPDIR=$TMPDIR
TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e
's,:$,,g'`
mkdir -p "$TMPDIR"
output=`"$@" 2>&1` || res=$?
+
+rmdir "$TMPDIR"
TMPDIR=$_TMPDIR
if [ "$res" -eq "$EXPECTEDRES" ]; then
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: make check as superuser leaves empty directories in /-directory
Hi,
i believe to have found out what's wrong with TMPDIR in
tests/grub_cmd_cryptomount.in
It does not get assigned a default value and is used without such a
value. Many other tests have the following gesture before using $TMPDIR:
: "${TMPDIR=/tmp}"
But grub_cmd_cryptomount.in has not and uses ${TMPDIR:-/tmp} only at a
single occasion when running mktemp. The mkdir step is without such a
safety net:
mkdir -p "$TMPDIR"
I fail to find this special expansion ${X=Y} in man bash, but experiments
show that it works like {X:=Y} with the difference that a defined empty X
is not filled with "Y".
(I do not think that this respecting of empty set variables is desirable
in GRUB tests. Empty TMPDIR is bad regardless of being set or unset.)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
make check as superuser leaves empty directories in /-directory
Hi, i find in the root directory of my system a lot of empty directories like /1678114331.LUKS1_test_with_twofish_cipher /1678114333.LUKS1_test_key_file_support I believe they come from tests/grub_cmd_cryptomount.in where i read eval testcase "'LUKS1 test with twofish cipher:'" \ @builddir@/grub-shell-luks-tester --luks=1 $COMMON_OPTS \ "--cs-opts='--cipher twofish-xts-plain64'" eval testcase "'LUKS1 test key file support:'" \ @builddir@/grub-shell-luks-tester --luks=1 $COMMON_OPTS \ --keyfile In the function _testcase() of that file i see mkdir -p "$TMPDIR" but the whole file contains no rmdir command. The path "$TMPDIR" is composed by # Create a subdir in TMPDIR for each testcase _TMPDIR=$TMPDIR TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'` So "$TMPDIR" would initially have been empty. I wonder why. I fail to see what else but mkdir is done to "$TMPDIR". So i cannot tell when it can be removed. "git blame" points to commit a7b540e6 by Glenn Washburn. (Cc'ed) The later commit 56b367d by Glenn Washburn is old enough to have been in effect when i first ran "make check" as superuser. Commit 5a311d0 by Gary Lin is too young for having caused my oldest /LUKS* directories. -- Why i think GRUB "make check" as superuser is to blame: The timestamps of the directories correlate with my memory of when i ran "make check" as superuser to check the impact of my proposed patch: https://lists.gnu.org/archive/html/grub-devel/2024-06/msg00197.html "[PATCH 0/2] grub-fstest: Show error message if command causes grub_errno" (The first paragraph of the cover letter is in https://lists.gnu.org/archive/html/grub-devel/2024-06/msg00201.html ) I ran "make check" as superuser last year with Debian 11 and this year with Debian 12. Both occasions obviously left their /*.LUKS* directories. A google search with "LUKS1_test_detached_header_support" leads to an earlier patch proposal by Glenn Washburn of 2020: https://www.mail-archive.com/grub-devel@gnu.org/msg30613.html Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fs/iso9660.c: Correct error message for missing UUID
Hi, sorry for goofing up "git send-email" again. This time not by omitting the empty line after the subject of a cover letter but probably by leaving the commit id of the previous "git format-patch" in the "git send-email" command line. git send-email --to=grub-devel@gnu.org $directory $surplus_id Shall i post the patch again as single message ? Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] fs/iso9660.c: Correct error message for missing UUID
The UUID emitted by function grub_iso9660_uuid is derived from the
ISO 9660 Primary Volume Descriptor field "Volume Modification Date and
Time". But the error message about possible invalid content of this
field talks of "creation date" rather than of "modification date".
Signed-off-by: Thomas Schmitt
---
grub-core/fs/iso9660.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 8c348b59a..a40c0fc0e 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -1166,7 +1166,7 @@ grub_iso9660_uuid (grub_device_t device, char **uuid)
&& ! data->voldesc.modified.second[0] && !
data->voldesc.modified.second[1]
&& ! data->voldesc.modified.hundredth[0] && !
data->voldesc.modified.hundredth[1])
{
- grub_error (GRUB_ERR_BAD_NUMBER, "no creation date in filesystem to
generate UUID");
+ grub_error (GRUB_ERR_BAD_NUMBER, "no modification date in filesystem
to generate UUID");
*uuid = NULL;
}
else
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] fs/iso9660.c: Correct error message for missing UUID
The UUID emitted by function grub_iso9660_uuid is derived from the
ISO 9660 Primary Volume Descriptor field "Volume Modification Date and
Time". But the error message about possible invalid content of this
field talks of "creation date" rather than of "modification date".
Signed-off-by: Thomas Schmitt
---
grub-core/fs/iso9660.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 8c348b59a..a40c0fc0e 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -1166,7 +1166,7 @@ grub_iso9660_uuid (grub_device_t device, char **uuid)
&& ! data->voldesc.modified.second[0] && !
data->voldesc.modified.second[1]
&& ! data->voldesc.modified.hundredth[0] && !
data->voldesc.modified.hundredth[1])
{
- grub_error (GRUB_ERR_BAD_NUMBER, "no creation date in filesystem to
generate UUID");
+ grub_error (GRUB_ERR_BAD_NUMBER, "no modification date in filesystem
to generate UUID");
*uuid = NULL;
}
else
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: How to test grub_iso9660_uuid from userland ?
Hi, the question is now: Is it worth to correct an error text which i cannot get to show up ? Vladimir 'phcoder' Serbinenko wrote: > grub-fstest IMAGE ls -- -l $ gunzip /tmp/iso9660_early_ce.iso $ ./grub-fstest /tmp/iso9660_early_ce.iso ls -- -l ... Device loop0: Filesystem type iso9660 - Label `ISOIMAGE' - Last modification time 2023-03-04 16:28:35 Saturday, UUID 2023-03-04-16-28-35-00 - Sector size 512B - Total size 68KiB ... Now zeroizing the modification date (offset 830 in PVD at offset 32768): $ dd if=/dev/zero of=/tmp/iso9660_early_ce.iso conv=notrunc bs=1 count=17 seek=33598 $ ./grub-fstest /tmp/iso9660_early_ce.iso ls -- -l ... Device loop0: Filesystem type iso9660 - Label `ISOIMAGE' - Sector size 512B - Total size 68KiB ... $ Well, it did not crash and there is no UUID. But i don't see the error message either. Switching to the branch with my proposed error text improvements of grub-fstest ... Still nothing to see. Obviously grub_errno is not set after function execute_command of grub-fstest.c. I checked that a bad ISO still causes a visible error message $ /grub-fstest /tmp/iso9660_ce_loop2.iso ls / -- -l ls : GRUB error 9 with message 'suspecting endless CE loop' ./grub-fstest: error: encountered 1 error during command execution. $ I also zeroized creation date at offset 32768+813 of the original ISO and verified that grub-fstest still shows an UUID. So the only thing i could prove by my test run is that indeed GRUB depends on ISO 9660 modification time for its UUID production. I failed to hack the function grub_error in grub-core/kern/err.c so that it shows grub_errmsg to me. Neither fprintf(3) nor write(2) seem in reach. The attempt to #include /usr/include/stdio.h or /usr/include/unistd.h fails with not found sub-header files. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
How to test grub_iso9660_uuid from userland ?
Hi, i found a wrong word in an error message of grub-core/fs/iso9660.c function grub_iso9660_uuid(): grub_error (GRUB_ERR_BAD_NUMBER, "no creation date in filesystem to generate UUID"); The missing entity would be the modification date data->voldesc.modified rather than the creation date data->voldesc.created The fix is obviously simple. But i have no idea how to get the code execution to this function for testing. An ISO with 0 date would be easy to fake. A use case known to me is in GRUB configuration with command "search" and option "--fs-uuid". In grub-fstest.c i see no opportunity to perform "search". Any idea how to test grub_iso9660_uuid() from userland of a running GNU/Linux ? Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] grub-fstest: Show error message if command causes grub_errno
Hi, somehow i managed to omit the first paragraph of the cover letter: - grub-fstest describes itself as "debug tool for filesystem driver" but fails to forward to the user the grub_errno status and grub_errmsg of the drivers under command "ls". Further it does not indicate such errors by its exit status. Reason is that function grub_cmd_ls() in grub-core/commands/ls.c unconditionally returns 0 and that grub_errno is not used anywhere in util/grub-fstest.c. - Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/2] grub-fstest: Show error message if command causes grub_errno
Most return values of function execute_command() are ignored in
util/grub-fstest.c. Only one of two "loopback" occasions and the command
"cryptomount" care. They perform grub_util_error() if the return value
is non-zero.
So there seems to be a general lack of handling of problems found in the
drivers or command implementations.
On the other hand, all tests which use grub-fstest are accustomed to this
silence about problems. Internal processing of grub-fstest does possibly
important things after an error occurs and before the program ends. It
seems inappropriate to immediately bail out when execute_command()
encounters grub_errno != GRUB_ERROR_NONE.
So i propose a patch which just counts the occurences of grub_errno in
execute_command(), reports grub_errmsg, and resets grub_errno.
Processing goes on as is tradition. But at the end of the program, a
non-zero error count causes a non-zero exit value.
Example:
$ gunzip /tmp/iso9660_ce_loop2.iso
$ ./grub-fstest /tmp/iso9660_ce_loop2.iso ls /
ls : GRUB error 9 with message 'suspecting endless CE loop'
./grub-fstest: error: encountered 1 error during command execution.
$ echo $?
1
(I will submit an addon patch which changes the grub_errno display to
ls : GRUB_ERR_BAD_FS with message 'suspecting endless CE loop'
by adding a new function grub_errno_to_name() to err.[ch].)
A new program option -E re-activates the current behavior.
I.e. it disables counting, reporting, and non-zero exit caused by
grub_errno.
After installing help2man and running ./configure i get from "make" a file
grub-fstest.1 which mentions the new option. (I fail to find file
grub-fstest.info which is promised by the text in grub-fstest.1.
Package texinfo is installed.)
I duely checked the impact of this change on "make check:
Installed are the Debian 12 packages which INSTALL prescribes.
"make check" was run as superuser with the old and the new version of
grub-fstest. Then i compared the outputs about PASS and FAIL.
The only test which newly failed is iso9660_ce_loop in
tests/iso9660_test.in because it takes non-zero exit as indication of
timeout, but will now regularly receive non-zero because
iso9660_ce_loop.iso is intentionally bad.
So patch 2/2 adds -E to the run of grub-fstest with iso9660_ce_loop.
This makes tests/iso9660_test PASS again.
4 tests yielded SKIP due to my platform "efi" and target "x86_64":
tests/fddboot_test.in
tests/netboot_test.in
tests/pseries_test.in
tests/core_compress_test.in
They are small and i can see no reference to grub-fstest in them.
6 tests yielded FAIL with both versions of grub-fstest. They seem
unrelated to grub-fstest:
- erofs_test says:
mkfs.erofs -Eforce-inode-compact -L g;/éт 莭😁 /tmp/grub-fs-tester.2024062018
0702880665192.erofs_compact.XeJ/erofs_compact_512_4096_1_0.img
/tmp/grub-fs-tester.20240620180702880665192.erofs_compact.XeJ/master
mkfs.erofs: invalid option -- 'L'
The test seems to expect something other than erofs-utils_1.5-1_amd64.deb
of Debian 12.
- exfat_test shows the help text of mkfs.exfat. In grub-fs-tester i see
x"exfat")
"mkfs.$fs" -s $((BLKSIZE/512)) -n "$FSLABEL" "${MOUNTDEVICE}"
But mkfs.exfat has neither option -s nor -n in help text or man page.
mkexfatfs offers: -n volume-name -s sectors-per-cluster
- hfs_test says:
LABEL FAIL
Device loop0: Filesystem type hfs - Label `untitled' ...
grub-fs-tester does:
FSLABEL="grub_t;/estéàèèéie fiucnree";;
"mkfs.hfs" -b $BLKSIZE -v "`echo $FSLABEL |recode utf8..macroman`" -h
"${MOUNTDEVICE}"
man mkfs.hfs shows man newfs_hfs, where -v is mentioned as:
-v volume-name
Volume name (file system name) in ascii or UTF-8 format.
(Was already so in Debian 11.)
So the recode filter seems unneeded and unhelpful.
- luks1_test says:
./grub-probe: error: disk
`lvm/grub-fs-tester.2023040443076506440.luks1.PDo' not found.
UUID FAIL
96efa19ce89c41279912b6a3b26a99d3
- luks2_test says:
./grub-probe: error: disk
`lvm/grub-fs-tester.2023040450278223161.luks2.NWP' not found.
UUID FAIL
8a1f1e83695d4dd1b6b73fcb542b73da
- grub_func_test reports checksum problems
tests/video_checksum.c:checksum:615: assert failed: 0 Checksum
cmdline_cat_2560x1440xrgba:44 failed: 0x62031fea vs 0x8071678a
---
Thomas Schmitt (2):
grub-fstest: Show error message if command causes grub_errno
tests: Use new grub-fstest option -E with iso9660_ce_loop test
tests/iso9660_test.in | 2 +-
util/grub-fstest.c| 30 +-
2 files changed, 30 insertions(+), 2 deletions(-)
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] tests: Use new grub-fstest option -E with iso9660_ce_loop test
This part of tests/iso9660_test works on an intentionally bad ISO which
with older versions of GRUB will cause an endless cycle. Therefore the
test runs grub-fstest under the program timeout. A non-zero exit value
is taken as indication that the timeout was triggered. Non-empty output
from grub-fstest is taken as indication that a CE entry was skipped by an
old bug, thus breaking the endless cycle by mistake.
A change in grub-fstest causes output of an error message and a non-zero
exit value when the bad ISO is handled properly.
So add the new grub-fstest option -E to the run of grub-fstest on
iso9660_ce_loop.iso and iso9660_ce_loop2.iso. This suppresses the error
message and the non-zero exit value.
Signed-off-by: Thomas Schmitt
---
tests/iso9660_test.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/iso9660_test.in b/tests/iso9660_test.in
index a1f752adf..677c777d9 100644
--- a/tests/iso9660_test.in
+++ b/tests/iso9660_test.in
@@ -20,7 +20,7 @@ for fs in iso9660_ce_loop iso9660_ce_loop2; do
{ echo "Failed to make temporary directory"; exit 99; }
gunzip <"$srcdir"/tests/${fs}.iso.gz >"${tempdir}/${fs}.iso" || exit 99
output=$(LC_ALL=C timeout -s KILL "60" \
-"@builddir@/grub-fstest" "${tempdir}/${fs}.iso" ls / ) || ret=$?
+"@builddir@/grub-fstest" -E "${tempdir}/${fs}.iso" ls / ) || ret=$?
rm -rf "$tempdir"
if [ "${ret:-0}" -ne 0 -o -n "$output" ]; then
echo "FAIL ($fs)"
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] grub-fstest: Show error message if command causes grub_errno
Check grub_errno after a command was performed. If not GRUB_ERR_NONE then
print grub_errno and grub_errmsg. Count the errors and exit 1 with a
message if the count is larger than zero when function fs_test() ends.
The number of printed messages is restricted to 3.
Introduce a new option -E which re-enables the old behavior which did
not show grub_errmsg and did not exit with non-zero value if grub_errno
was found not being GRUB_ERR_NONE.
Signed-off-by: Thomas Schmitt
---
util/grub-fstest.c | 30 +-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index 7ff9037b8..145b40a0e 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -47,16 +47,33 @@
#pragma GCC diagnostic error "-Wmissing-prototypes"
#pragma GCC diagnostic error "-Wmissing-declarations"
+static int execute_error_count = 0;
+static int no_cmd_error = 0;
+
static grub_err_t
execute_command (const char *name, int n, char **args)
{
grub_command_t cmd;
+ grub_err_t ret;
cmd = grub_command_find (name);
if (! cmd)
grub_util_error (_("can't find command `%s'"), name);
- return (cmd->func) (cmd, n, args);
+ ret = (cmd->func) (cmd, n, args);
+ if (grub_errno != GRUB_ERR_NONE && !no_cmd_error)
+{
+ execute_error_count++;
+ if (execute_error_count <= 3)
+ grub_err_printf (_("%s : GRUB error %d with message '%s'\n"),
+name, (int) grub_errno, grub_errmsg);
+ else if (execute_error_count == 4)
+ grub_err_printf (_("%s : more GRUB errors encountered but not shown\n"),
+name);
+ grub_errno = GRUB_ERR_NONE;
+ grub_errmsg[0] = 0;
+}
+ return ret;
}
enum {
@@ -510,6 +527,12 @@ fstest (int n)
grub_free (loop_name);
grub_free (argv[0]);
}
+
+ if (execute_error_count == 1)
+grub_util_error (_("encountered 1 error during command execution"));
+ else if (execute_error_count > 0)
+grub_util_error (_("encountered %d errors during command execution"),
+execute_error_count);
}
static struct argp_option options[] = {
@@ -535,6 +558,7 @@ static struct argp_option options[] = {
N_("FILE|prompt"), 0, N_("Load zfs crypto key."), 2},
{"verbose", 'v', NULL, 0, N_("print verbose messages."), 2},
{"uncompress", 'u', NULL, 0, N_("Uncompress data."), 2},
+ {"no-cmd-error", 'E', NULL, 0, N_("Ignore errors of command execution."), 2},
{0, 0, 0, 0, 0, 0}
};
@@ -638,6 +662,10 @@ argp_parser (int key, char *arg, struct argp_state *state)
uncompress = 1;
return 0;
+case 'E':
+ no_cmd_error = 1;
+ return 0;
+
case ARGP_KEY_END:
if (args_count < num_disks)
{
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/1] util/grub-mkrescue: Check existence of option arguments
As reported by Victoriia Egorova in bug 65880, grub-mkrescue does not
verify that the expected argument of an option like -d or -k does really
exist in argv.
So check the loop counter before incrementing it inside the loop which
copies argv to argp_argv. Issue an error message similar to what older
versions of grub-mkrescue did with a missing argument (e.g 2.02).
Fixes: https://savannah.gnu.org/bugs/index.php?65880
Signed-off-by: Thomas Schmitt
---
util/grub-mkrescue.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
index abcc1c2f5..8714d819e 100644
--- a/util/grub-mkrescue.c
+++ b/util/grub-mkrescue.c
@@ -477,6 +477,9 @@ main (int argc, char *argv[])
for (i = 1; i < argc; i++)
{
if (strcmp (argv[i], "-output") == 0) {
+ if (i + 1 >= argc)
+ grub_util_error ("%s -- '%s'", _("option requires an argument"),
+argv[i]);
argp_argv[argp_argc++] = (char *) "--output";
i++;
argp_argv[argp_argc++] = argv[i];
@@ -485,6 +488,9 @@ main (int argc, char *argv[])
switch (args_to_eat (argv[i]))
{
case 2:
+ if (i + 1 >= argc)
+ grub_util_error ("%s -- '%s'", _("option requires an argument"),
+ argv[i]);
argp_argv[argp_argc++] = argv[i++];
/* Fallthrough */
case 1:
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] util/grub-mkrescue: use capitalised paths for removable EFI images
Hi,
now that i know how to test grub-mkrescue out of the git clone, i also
gave your patch v2 a run.
Its mail form seems to be problematic:
- Alpine shows two leading blanks in the context lines instead of one.
Empty context lines show no leading blank. Long lines show intermediate
line breaks (which is a normal behavior for alpine).
- But running the blob in your raw mail body through "base64 -d" yields
the same, including the surplus line breaks.
- In https://lists.gnu.org/archive/html/grub-devel/2024-06/msg00101.html
the changed -/+ line pairs are shown merged to single lines. Blanks are
missing there.
- After removing the surplus blanks, inserting the missing ones, and
merging lines, i still don't get the second hunk through "patch -p1 -b"
until i insert
@@ -818,11 +818,11 @@ main (int argc, char *argv[])
imgname);
after
make_image_fwdisk_abs (GRUB_INSTALL_PLATFORM_LOONGARCH64_EFI,
"loongarch64-efi",
imgname);
(One may blame this on my limited patch knowledge which is inviting
witchcraft.)
Now patch works in the current git clone with these messages:
patching file util/grub-mkrescue.c
Hunk #1 succeeded at 767 (offset -2 lines).
Hunk #2 succeeded at 790 with fuzz 2 (offset -2 lines).
Hunk #3 succeeded at 818 with fuzz 1.
Compiling works without complaints.
I successfully built grub-mkrescue and tested ISO production with
platform={efi,pc} x target={i386,x86_64}
The "efi" test runs used the xorriso options which would have caused a
failure with the patch v1. (I tested yesterday that it indeed would
happen with the ISO/HFS+ file name change if not the option was changed
accordingly.)
The ISOs of all 4 runs showed the expected boot lures. I did not test
whether they actually boot, though. It would not help too much, because
in the end the most hunchbacked of the really existing firmwares decides
about the usability of the names.
So half a "Tested-by:" for the patch ...
and a "Boooh !" towards your mail program.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[Solved] Re: How to test the git clone without "make install" ?
Hi,
i wrote:
> > So i will start a new thread with the question:
> > How do i convince the git clone to produce programs and ISO for 64 bit
> > EFI.
Vladimir 'phcoder' Serbinenko wrote:
> ./configure --with-platform=efi --target=x86_64
Fast and concise as ever. :))
Thanks, Vladimir.
It works if i do beforehand
make clean
Without "make clean" i get from the subsequent "make":
symlist.h:30:10: fatal error: ../include/grub/machine/pxe.h: No such file or
directory
30 | #include <../include/grub/machine/pxe.h>
Trying to revoke the change by
./configure && make
yields:
symlist.c: In function ‘grub_register_exported_symbols’:
symlist.c:66:31: error: ‘grub_acpi_find_fadt’ undeclared (first use in this
function)
66 | {"grub_acpi_find_fadt", grub_acpi_find_fadt, 1},
---
Well, now that i know the magic words, i find them in the manual under
"8 Porting".
Possibly it would be worthwhile to put some of this into a new chapter
"1.1 Building from the git clone"
together with a warning about re-configuring without clean-making.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[Solved] Re: How to test the git clone without "make install" ?
Hi, Maximilian Stendler wrote: > to keep the host installation clean, I would probably use a container. Yes, a virtual machine came to my mind. Easy to clone and to dispose. But there must be some better way to test a utility built from git independenly of systemwide directories. Vladimir 'phcoder' Serbinenko wrote: > Set pkgdatadir environment variable Ahum ... rm /usr/local/share/grub pkgdatadir=. ./grub-mkrescue -o /dvdbuffer/test.iso yields indeed an ISO with EFI boot equipment. But what to do about /usr/local/lib/grub ? I found option -d meanwhile. After some forth and back i came to pkgdatadir=. ./grub-mkrescue -o /dvdbuffer/test.iso -d ./grub-core which to my surprise creates an ISO with boot equipment for legacy BIOS: $ xorriso -indev /dvdbuffer/test.iso -report_el_torito plain -report_system_area plain ... El Torito images : N Pltf B Emul Ld_seg Hdpt Ldsiz LBA El Torito boot img : 1 BIOS y none 0x 0x00 41397 El Torito img path : 1 /boot/grub/i386-pc/eltorito.img El Torito img opts : 1 boot-info-table grub2-boot-info ... System area summary: MBR protective-msdos-label grub2-mbr cyl-align-off ... MBR partition table: N Status TypeStart Blocks MBR partition : 1 0x80 0xcd113783 While i used the Debian system directories it was EFI: El Torito images : N Pltf B Emul Ld_seg Hdpt Ldsiz LBA El Torito boot img : 1 UEFI y none 0x 0x00 5760 52 El Torito img path : 1 /efi.img ... System area summary: MBR protective-msdos-label cyl-align-off GPT APM ... MBR partition table: N Status TypeStart Blocks MBR partition : 1 0x00 0xee118015 ... and a GPT and an Apple Partition Map for HFS+ ... So i will start a new thread with the question: How do i convince the git clone to produce programs and ISO for 64 bit EFI. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
How to test the git clone without "make install" ?
Hi, on occasion of https://savannah.gnu.org/bugs/index.php?65880 "heap-buffer-overflow in grub-mkrescue.c" i try to get grub-mkrescue running from git. My problem is that grub_util_get_pkglibdir() returns /usr/local/lib/grub and grub_util_get_pkgdatadir() returns /usr/local/share/grub which of course do not come with a Debian installation. So grub-mkrescue produces only a very small ISO with no boot lures or boot programs. Quite unrealistic for testing. I was able to overcome this obstacle by ln -s /usr/lib/grub /usr/local/lib/grub ln -s /usr/share/grub /usr/local/share/grub but i understand that now my grub-mkrescue actually copies the ISO content from the Debian installation and not from the git clone. The manual https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html does not give me a clue how i would get the /usr/local/*/grub directories populated with the files made from the git clone. I guess "make install" would do it for me, but i fear that this does too many other things to the GRUB installation of my vanilla Debian. In general i would prefer to keep the git files away from any system directory. So what can i do to make the files built from git available to ./grub-mkrescue built from git, without frankensteining my Debian 12 ? Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] util/grub-mkrescue: use capitalised paths for removable EFI images
Hi, Mingcong Bai wrote: > I was testing for loongarch64-efi. As noted in the commit message, I > found that Loongson's firmware incapable of handling non-upper-case EFI > boot paths If no test reports emerge about other platforms, then i would consider to reduce the patch to what was tested and is really needed with real-world firmware. The potential xorriso failure would only show up with x86 platforms. So reducing the patch to the smallest needed change would be the best way to avoid such mishaps with untested platforms. > with the boot.efi change removed, there shouldn't be any more > error - right? I see no other change in your patch which would influence the ISO 9660 filesystem tree and the possible HFS+ tree. Thus i think it should be safe. Note that the name "boot.efi" gets used for two occasions: Once it gets into the FAT filesystem and once into the ISO 9660 / HFS+ filesystem. The comments in the code and my conversations with Vladimir Serbinenko indicate that both times it is for x86 Macs. Those which read HFS+ are probably quite old. The FAT filesystem image becomes a data file in the ISO filesystem. xorriso will not peek into the FAT filesystem. So renaming the "boot.efi" which goes into FAT will not cause the need for changes about the xorriso options. (File names in the ISO 9660 fileystem are case-insensitive for the dull ISO names, but case-sensitive for the Rock Ridge extensions which augment the ISO 9660 filesystem to comply with X/Open specs. The HFS+ tree, which shares data file content with the ISO tree, is case-insensitive. Since the "xo" in the name "xorriso" stands for X/Open and the "rr" stands for Rock Ridge, xorriso is case-sensitive with its file path arguments.) > > - The new name "BOOTx64.EFI" isn't actually all uppercase. > Indeed, but I'm no one to argue with the specifications. Avoiding to change it would elegantly avoid the question whether we know better than the specs. Else you would possibly have to change the patch title to something like: "use paths for removable EFI images as prescribed by UEFI" > non-upper-case EFI boot paths (which grub-install happens to evade, > as it creates upper-case paths and filenames in the first place) So you could call the patch a unification of name habits between grub-install and grub-mkrescue. Maybe this attracts more testers for the other platforms. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] util/grub-mkrescue: use capitalised paths for removable EFI images
Hi, Mingcong Bai wrote: > Thanks for your review. I will now submit v2. Well, it's not actually a review but rather pointing out a problem which would probably cause a failure of the xorriso run, when the option -hfs-bless-by i /System/Library/CoreServices/boot.efi does not find "boot.efi" because it would now be named "BOOT.EFI". I'd expect this message and no resulting ISO image: xorriso : FAILURE : Cannot find path '/System/Library/CoreServices/boot.efi' in loaded ISO image I'm only qualified for judging the xorriso aspect of grub-mkrescue. The EFI aspect is mostly out of my scope. So i can't add a "Reviewed-by:". Two more thoughts which came to me since monday: - Which of the platforms did you actually test ? (I would be astonished if GRUB_INSTALL_PLATFORM_I386_EFI and GRUB_INSTALL_PLATFORM_X86_64_EFI were among them, because else xorriso should have thrown above error.) - The new name "BOOTx64.EFI" isn't actually all uppercase. I am aware that the specs indeed mention the name that way in UEFI 2.8 table 15. But that table is obviously buggy by calling in column 1 all three RISC-V architectures "32-bit". So the lowercase "x" in "BOOTx64.EFI" might be an old typo in the specs. (It's already present in UEFI 2.4 table 12.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] util/grub-mkrescue: use capitalised paths for removable EFI images
Hi,
Mingcong Bai wrote:
> While FAT < 32 filesystems are not case sensitive (which grub-mkrescue
> creates
> as a FAT12 image via mformat with a size of 2.88MiB), it seems that
> some of Loongson's LoongArch-based firmware (namely those found on their
> latest XA61200 boards) seems to treat this file system as
> case-sensitive.
> [...]
> if (source_dirs[GRUB_INSTALL_PLATFORM_I386_EFI]
> || source_dirs[GRUB_INSTALL_PLATFORM_X86_64_EFI])
> - img_mac = grub_util_path_concat (2, core_services, "boot.efi");
> + img_mac = grub_util_path_concat (2, core_services, "BOOT.EFI");
>
> if (source_dirs[GRUB_INSTALL_PLATFORM_I386_EFI]
> && source_dirs[GRUB_INSTALL_PLATFORM_X86_64_EFI])
I think that this name does not end up in a FAT filesystem but rather
in HFS+, where it gets blessed. (It also ends up in ISO 9660.)
707: core_services = grub_util_path_concat (4, iso9660_dir, "System",
"Library", "CoreServices");
753: xorriso_push ("-hfs-bless-by");
754: xorriso_push ("i");
755: xorriso_push ("/System/Library/CoreServices/boot.efi");
This xorrisofs option would not reach a file in the EFI boot image.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] util/grub-mkrescue: specify -iso-level 3 in xorriso options
Hi, Gerd Hoffmann wrote: > I doubt the vfat driver of the uefi firmware supports rock ride, > and that is the driver used to load efi/boot/bootloongarch64.efi > because grub is not yet loaded ... xorrisofs option -iso-level 3 has no effect on the FAT filesystem in the EFI boot image (/efi.img in the grub-mkrescue ISO). xorriso takes this file as opaque input and just advertises it to the firmware. Whether grub-mkrescue can represent "bootloongarch64.efi" in FAT is a matter of the helper programs "mformat" and "mcopy". See line 845 ff. of util/grub-mkrescue.c . Line 813 ff. shows that it already uses long names "bootriscv32.efi" and "bootriscv64.efi The effect of xorrisofs option -iso-level is descibed in the man page of xorrisofs: -iso-level number Specify the ISO 9660 version which defines the limitations of file naming and data file size. The naming restrictions do not apply to the Rock Ridge names but only to the low-level ISO 9660 names. There are three conformance levels: Level 1 allows ISO names of the form 8.3 and file size up to 4 GiB - 1. Level 2 allows ISO names with up to 32 characters and file size up to 4 GiB - 1. Level 3 allows ISO names with up to 32 characters and file size of up to 400 GiB - 200 KiB. (This size limitation is set by the xorriso implementation and not by ISO 9660 which would allow nearly 8 TiB.) Pseudo-level 4 enables production of an additional ISO 9660:1999 directory tree. Levels 1 to 3 are specified in ECMA-119, 10 "Levels of interchange". Pseudo-level 4 is an invention of mkisofs, although it would have been better to give the ISO 9660:1999 tree an own option, like for Joliet. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] util/grub-mkrescue: specify -iso-level 3 in xorriso options
Hi, Mingcong Bai wrote: > This is needed for architectures whose EFI executables has file names that > exceeds the 8.3 (extension.suffix) convention, such as LoongArch64 > (bootloongarch64.efi). In general -iso-level 3 is a harmless setting if the reading operating system is Linux or MS-Windows. On other reading system your mileage may vary. In any case level 3 is harmless if it is not actually needed, i.e. all file names are 8.3 and all files are smaller than 4 GiB. But i wonder: - Why level 3 and not 2 ? If it is only about file name length, then -iso-level 2 would suffice and not enable file sizes of 4 GiB and larger. Such files have to be represented by more than one extent, which might be not supported by the BSDs and other systems. Especially in grub-core/fs/iso9660.c function grub_iso9660_read() i see a remark: "XXX: The file is stored in as a single extent" and cannot spot provisions to read more than one directory entry per file. So i doubt that GRUB is prepared for multi-extent files. (If somebody wants to test: libcdio has a small ISO image https://git.savannah.gnu.org/cgit/libcdio.git/tree/test/data/multi_extent_8k.iso of size 122 KB with artificially small extent size. So its file /multi_extent_file bears 8 extents although it has only 54 KB of size. Compare GRUB's representation of the file with that of GNU/Linux.) - Why is the name bootloongarch64.efi a problem in the ISO filesystem ? The name length restriction of -iso-level 1 applies only to the dull ISO 9660 names, but not to Rock Ridge names, which GRUB uses if present. (Other than mkisofs or genisoimage, xorriso produces Rock Ridge by default. Just don't use option --norock.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] fs/iso9660: Delay CE hop until end of current SUSP area
Hi, Daniel Kiper wrote: > AIUI this [1] email presented some concerns. > [1] https://lists.gnu.org/archive/html/grub-devel/2023-03/msg00022.html This was about the trailing blank in the output of grub-fstest. I decided to simply remove it because it is not expectable that any wrong handling of a CE entry would produce blanks from the two filename alternatives "ROCKRIDG.;1" and "RockRidgeName:x". So i wrote in [PATCH 2/2]: +# Before comparing: remove trailing blank added by grub-fstest +output=$(echo -n $output) (I remember to have found the origin of the blank somewhere in the deeper levels of the code, possibly grub-core/commands/ls.c function print_files. It did not look like it would be reasonable to ask for changing it.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] tests: Add test for iso9660 delayed CE hop
The ISO filesystem image iso9660_early_ce.iso exposes the unusual
situation that the Rock Ridge name entry of its only file is located
after a CE entry which points to the next continuation area.
The correct behavior is to read the Rock Ridge name and to only then
load the next continuation area. If GRUB performs this correctly, then
the name "RockRidgeName:x" will be read and reported by grub-fstest.
If GRUB wrongly performs the CE hop immediatly when encountering the CE
entry, then the dull ISO 9660 name "rockridg" will not be overridden and
be put out by grub-fstest.
Signed-off-by: Thomas Schmitt
---
tests/iso9660_early_ce.iso.gz | Bin 0 -> 709 bytes
tests/iso9660_test.in | 24
2 files changed, 24 insertions(+)
create mode 100644 tests/iso9660_early_ce.iso.gz
diff --git a/tests/iso9660_early_ce.iso.gz b/tests/iso9660_early_ce.iso.gz
new file mode 100644
index
..df4ef5fb276944c41a8e355b96423ce7e935d585
GIT binary patch
literal 709
zcmb2|=3wyXVNPLSetW~$U)WLNz(+~1E!S8$9vm%@xUx8Jo9`Z$|g_8lRLub~f&3bja#OVIfzS`OkVU;;7brX~~Lbo}q6-
z-PF_Gbx-{C%W?C+`?)UH(#oF&-p#bsy;>mcdpy^o*LC^5BJn#v+qDc%NB%Cl-sU=g
z)3Uj9wC0=eSKI#I@rSu=_PdzceH(A&>|AFN{_x+UnpTm+ajb2xjq0{FdKccg|DTcJ
zz}JtN4~)x%?oWx|-&ygv(dN@9y?cTGcNDz4GGW{MQyc$%O&5+oe`4jL%ON*^yU((ZsWrmxGKn%|*yUvR5Y;;Oa#pS4EXZ3~q888{*R+Sx9@kW0OfPb^y{
z8!77Ec{3naIAdy#+RAq;T*~(tnAQog-}2t_X|ciAKOt*p90@Nhw=_68Yun!L$q~Po
z<<{t%2CkU2BD?nO3Y{jqtA^4>+}F)2&CaMLTFtu~@;dOs+l;Bc$IpvRbaOpWq$8ZR
z)K7oSV^-mtX)%(uzvFt+mUkuVTb$#wFxQHfFDtX_dY`xV;+*WW`Z{{C{d2^7ybFV5
xzJ&`i6tt$izd8FqFqn7vRayU6VJ9ZN=p}78dn3uf(6IWO@akK61q=d=3;-EpKr#RT
literal 0
HcmV?d1
diff --git a/tests/iso9660_test.in b/tests/iso9660_test.in
index 44bc08c6d..d7022e061 100644
--- a/tests/iso9660_test.in
+++ b/tests/iso9660_test.in
@@ -28,3 +28,27 @@ for fs in iso9660_ce_loop iso9660_ce_loop2; do
fi
done
echo "PASS"
+
+echo "Testing for proper handling of early CE ... "
+fs=iso9660_early_ce
+tempdir=`mktemp -d "${TMPDIR:-/tmp}/${0##*/}.$(date
'+%Y%m%d%H%M%S%N').${fs}.XXX"` ||
+ { echo "Failed to make temporary directory"; exit 99; }
+gunzip <"$srcdir"/tests/${fs}.iso.gz >"${tempdir}/${fs}.iso" || exit 99
+ret=0
+output=$(LC_ALL=C timeout -s KILL "60" \
+ "@builddir@/grub-fstest" "${tempdir}/${fs}.iso" ls / ) || ret=$?
+rm -rf "$tempdir"
+if [ "${ret:-0}" -ne 0 ]; then
+ echo "... grub-fstest returns $ret"
+ echo "FAIL ($fs)"
+ exit 1
+fi
+# Before comparing: remove trailing blank added by grub-fstest
+output=$(echo -n $output)
+if [ x"$output" != x"RockRidgeName:x" ]; then
+ echo "... found: '$output' , expected: 'RockRidgeName:x'"
+ echo "FAIL ($fs)"
+ exit 1
+fi
+echo "PASS"
+
--
2.30.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] fs/iso9660: Delay CE hop until end of current SUSP area
The SUSP specs demand that the reading of the next SUSP area which is
depicted by a CE entry shall be delayed until reading of the current
SUSP area is completed.
Up to now GRUB immediately ends reading of the current area and loads the
new one.
So buffer the parameters of a found CE entry and perform checks and
reading of new data only after the reader loop has ended.
Signed-off-by: Thomas Schmitt
---
grub-core/fs/iso9660.c | 84 ++
1 file changed, 44 insertions(+), 40 deletions(-)
diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index acccf5fc7..adf960771 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -272,6 +272,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
struct grub_iso9660_susp_entry *entry;
grub_err_t err;
int ce_counter = 0;
+ grub_ssize_t ce_sua_size = 0;
+ grub_off_t ce_off;
+ grub_disk_addr_t ce_block;
if (sua_size <= 0)
return GRUB_ERR_NONE;
@@ -293,6 +296,7 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
entry = (struct grub_iso9660_susp_entry *) sua;
+ next_susp_area:
while (entry->len > 0)
{
/* Ensure the entry is within System Use Area. */
@@ -307,55 +311,21 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
{
struct grub_iso9660_susp_ce *ce;
- grub_disk_addr_t ce_block;
- if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+ if (ce_sua_size > 0)
{
grub_free (sua);
return grub_error (GRUB_ERR_BAD_FS,
-"suspecting endless CE loop");
+"more than one CE entry in SUSP area");
}
+ /* Buffer CE parameters for use after the end of this loop. */
ce = (struct grub_iso9660_susp_ce *) entry;
- sua_size = grub_le_to_cpu32 (ce->len);
- off = grub_le_to_cpu32 (ce->off);
+ ce_sua_size = grub_le_to_cpu32 (ce->len);
+ ce_off = grub_le_to_cpu32 (ce->off);
ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
-
- if (sua_size <= 0)
- break;
-
- if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
- {
- grub_free (sua);
- return grub_error (GRUB_ERR_BAD_FS,
-"invalid continuation area in CE entry");
- }
-
- grub_free (sua);
- sua = grub_malloc (sua_size);
- if (!sua)
- return grub_errno;
-
- /* Load a part of the System Usage Area. */
- err = grub_disk_read (node->data->disk, ce_block, off,
- sua_size, sua);
- if (err)
- {
- grub_free (sua);
- return err;
- }
-
- entry = (struct grub_iso9660_susp_entry *) sua;
- /*
- * The hook function will not process CE or ST.
- * Advancing to the next entry would skip them.
- */
- if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
- || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
- continue;
}
-
- if (hook (entry, hook_arg))
+ else if (hook (entry, hook_arg))
{
grub_free (sua);
return 0;
@@ -367,6 +337,40 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
break;
}
+ if (ce_sua_size > 0)
+{
+ /* Load the next System Use Area by buffered CE entry parameters. */
+ if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+ {
+ grub_free (sua);
+ return grub_error (GRUB_ERR_BAD_FS, "suspecting endless CE loop");
+ }
+ if (ce_sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+ {
+ grub_free (sua);
+ return grub_error (GRUB_ERR_BAD_FS,
+"invalid continuation area in CE entry");
+ }
+
+ grub_free (sua);
+ sua = grub_malloc (ce_sua_size);
+ if (!sua)
+ return grub_errno;
+
+ err = grub_disk_read (node->data->disk, ce_block, ce_off, ce_sua_size,
+ sua);
+ if (err)
+ {
+ grub_free (sua);
+ return err;
+ }
+ entry = (struct grub_iso9660_susp_entry *) sua;
+ sua_size = ce_sua_size;
+ ce_sua_size = 0;
+
+ goto next_susp_area;
+}
+
grub_free (sua);
return 0;
}
--
2.30.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/2] fs/iso9660: Delay CE hop until end of current SUSP area
Hi, SUSP 1.12 says: The "CE" System Use Entry indicates a Continuation Area that shall be processed after the current System Use field or Continuation Area is processed. But GRUB rather takes an encountered CE entry as reason to immediately switch reading to the location that is given by the CE entry. This can skip over important information. The usual ISO 9660 producers on GNU/Linux write the CE entry as last entry of System Use field or Continuation Area. So the problem does not show up with their output. Nevertheless, Linux and libisofs obey the specs whereas GRUB does not. As demonstration i crafted a small ISO, where the CE entry comes before the NM entry which tells the Rock Ridge file name "RockRidgeName:x". Linux shows the NM name, nevertheless: $ sudo mount iso9660_early_ce.iso /mnt/iso mount: /mnt/iso: WARNING: source write-protected, mounted read-only. $ ls /mnt/iso RockRidgeName:x $ GRUB does not see the NM entry and thus shows the dull ISO 9660 name (which is actually "ROCKRIDG.;1"): $ ./grub-fstest iso9660_early_ce.iso ls / rockridg $ After the code change of my patch, i get: $ ./grub-fstest iso9660_early_ce.iso ls / RockRidgeName:x $ A new code block in tests/iso9660_test.in verifies that the patched code is in effect: make check TESTS=iso9660_test detects the old code state and shows that the new code still has the capability to cope with endless CE loops. - How to create an ISO 9660 filesystem where CE is not the last SUSP entry of a file's directory record: # Deliberately chosen names iso=iso9660_early_ce.iso # rr_path is longer than 8, mixed-case, with non-ISO-9660 character rr_path=/RockRidgeName:x # A dummy file as payload echo x >x # 250 fattr characters to surely exceed the size of a directory record long_string=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789 # Create ISO with the payload file and attached fattr named user.dummy . # Make it small with the most restrictive ISO 9660 file name rules. test -e "$iso" && rm "$iso" xorriso -compliance no_emul_toc:iso_9660_level=1 \ -padding 0 \ -outdev "$iso" \ -xattr on \ -map x "$rr_path" \ -setfattr user.dummy "$long_string" "$rr_path" -- # Cut out the NM field and the CE field from the directory record # of $rr_path. The numbers were determined by looking at a hex dump. dd if="$iso" bs=1 skip=37198 count=20 of=nm_field dd if="$iso" bs=1 skip=37218 count=28 of=ce_field # Put them back in reverse sequence dd conv=notrunc if=ce_field bs=1 seek=37198 of="$iso" dd conv=notrunc if=nm_field bs=1 seek=37226 of="$iso" - Have a nice day :) Thomas Thomas Schmitt (2): fs/iso9660: Delay CE hop until end of current SUSP area tests: Add test for iso9660 delayed CE hop grub-core/fs/iso9660.c| 84 ++ tests/iso9660_early_ce.iso.gz | Bin 0 -> 709 bytes tests/iso9660_test.in | 24 ++ 3 files changed, 68 insertions(+), 40 deletions(-) create mode 100644 tests/iso9660_early_ce.iso.gz -- 2.30.2 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Is the last blank in the output of grub-fstester a reliable feature ?
Hi,
i am working on a patch to let GRUB delay the execution of a ISO 9660
SUSP CE hop until the current SUSP area is consumed. Currently it hops
immediately which is wrongly relying on implementation details of the
ISO producer. SUSP 1.12 specs are clear. Linux obeys them.
The code works but the test makes problems because grub-fstest adds a
blank to the filename. That's probably a preparation for the next name
to be listed and a bit inappropriate with the last and only name of the
output list.
Is this behavior of grub-fstest reliable ?
Do other tests already handle such blanks ? If so: how ?
May i expect the trailing blank in the test ? Like:
# grub-fstest adds a blank to the name. So expect that blank too.
if [ "${ret:-0}" -ne 0 -o x"$output" != x"RockRidgeName:x " ]; then
I fail to find the answers in ./grub-fs-tester .
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] tests: Add pathological iso9660 filesystem tests
Hi, Glenn Washburn wrote: > > > +echo "Testing for proper recognition of CE loops ... " > Ugh, this should be "echo -n". My make "check" wrote: > > Testing for proper recognition of CE loops ... > > FAIL (iso9660_ce_loop) > > FAIL iso9660_test (exit status: 1) Actually this looks quite ok to me. With echo -n "Testing for proper recognition of CE loops ... " and a disabled loop breaker i get in test-suite.log: Testing for proper recognition of CE loops ... FAIL (iso9660_ce_loop) FAIL iso9660_test (exit status: 1) which looks to me like a newline is missing. The combination of "echo -n" and "..." seems not to occur in the GRUB code: $ grep -r 'echo -n .*\.\.\.' . | less Some echo without -n have "...": $ grep -r 'echo .*\.\.\.' . | wc -l 65 $ grep -r 'echo .*\.\.\.' tests tests/partmap_test.in:echo "Checking MSDOS partition types..." tests/partmap_test.in:echo "Checking GPT partition types..." tests/partmap_test.in:echo "Checking SUN partition types..." tests/partmap_test.in:echo "Checking APPLE partition types..." tests/partmap_test.in:echo "Checking DVH partition types..." tests/partmap_test.in:echo "Checking AMIGA partition types..." tests/partmap_test.in:echo "Checking DragonFly BSD disklabel64..." $ If "..." as line end is too near to ./configure style, then how about echo "Testing for proper recognition of CE loops:" Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] docs: Document that the functional test requires the package xfonts-unifont
Hi, On Thu, 16 Feb 2023 01:15:01 -0600, Glenn Washburn wrote: > Document that the functional test requires the package xfonts-unifont > > Signed-off-by: Glenn Washburn > --- > INSTALL | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/INSTALL b/INSTALL > index 620dcceb48..d1d5a6b88a 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -70,6 +70,7 @@ Prerequisites for make-check: > * xorriso 1.2.9 or later, for grub-mkrescue and grub-shell > * wamerican, for grub-fs-tester > * mtools, FAT tools for EFI platforms > +* xfonts-unifont, for the functional tests > > * If running a Linux kernel the following modules must be loaded: >- fuse, loop Reviewed-by: Thomas Schmitt Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] tests: Return hard error for functional test when unicode.pf2 does not exist
Hi,
On Thu, 16 Feb 2023 01:15:00 -0600, Glenn Washburn
wrote:
> The functional test requires unicode.pf2 to run successfully, so
> explicitly have the test return ERROR when its not found.
>
> Signed-off-by: Glenn Washburn
> ---
> tests/grub_func_test.in | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/tests/grub_func_test.in b/tests/grub_func_test.in
> index c67f9e4225..d43427c568 100644
> --- a/tests/grub_func_test.in
> +++ b/tests/grub_func_test.in
> @@ -3,6 +3,11 @@ set -e
>
> . "@builddir@/grub-core/modinfo.sh"
>
> +if [ ! -e "@builddir@/"unicode.pf2 ]; then
> + echo "Functional test requires grub-mkfont support"
> + exit 99
> +fi
> +
> case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
> # PLATFORM: Max RAM is 256M
> mips-qemu_mips | mipsel-qemu_mips)
Tested-by: Thomas Schmitt
With xfonts-unifont installed, the test gets beyond the grub-mkisofs run
and shows the usual complaints about checksums.
After
apt-get remove xfonts-unifont
and in the git repo:
rm unicode.pf2
./configure && make
the grub_func_test ends very quickly with:
Functional test requires grub-mkfont support
ERROR grub_func_test (exit status: 99)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] tests: Add pathological iso9660 filesystem tests
ub-fs-tester" joliet_1999
> "@builddir@/grub-fs-tester" rockridge_1999
> -"@builddir@/grub-fs-tester" rockridge_joliet_1999
> \ No newline at end of file
> +"@builddir@/grub-fs-tester" rockridge_joliet_1999
> +
> +echo "Testing for proper recognition of CE loops ... "
> +for fs in iso9660_ce_loop iso9660_ce_loop2; do
> + tempdir=`mktemp -d "${TMPDIR:-/tmp}/${0##*/}.$(date
> '+%Y%m%d%H%M%S%N').${fs}.XXX"` ||
> +{ echo "Failed to make temporary directory"; exit 99; }
> + gunzip <"$srcdir"/tests/${fs}.iso.gz >"${tempdir}/${fs}.iso" || exit 99
> + output=$(LC_ALL=C timeout -s KILL "60" \
> +"@builddir@/grub-fstest" "${tempdir}/${fs}.iso" ls / ) || ret=$?
> + rm -rf "$tempdir"
> + if [ "${ret:-0}" -ne 0 -o -n "$output" ]; then
> +echo "FAIL ($fs)"
> +exit 1
> + fi
> +done
> +echo "PASS"
Tested-by: Thomas Schmitt
The test passes with unchanged grub-core/fs/iso9660.c.
It fails with disabled loop breaker and with disabled processing of CE at
the start of a continuation area.
The message in test-suite.log is then:
Testing for proper recognition of CE loops ...
FAIL (iso9660_ce_loop)
FAIL iso9660_test (exit status: 1)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: make check: grub-mkrescue fails missing unicode.pf2. Leftovers in /tmp
Hi, another confused statement of mine: > Unicode is mentioned two times in INSTALL. I meant "Unifont" and "unifont". Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: make check: grub-mkrescue fails missing unicode.pf2. Leftovers in /tmp
Hi, Glenn Washburn wrote: > IIRC, xfonts-unifont is pulled in by the unifont package on Debian, but > it is the one with the files you need. No, its not documented, but then > not every single debian package name needed is documented because the > documentation is partially distribution agnostic. If it were documented > it would be in ./INSTALL, which does talk about needing unifonts. If > you'd like to modify that file to help other Debian users, I'm > supportive. Unicode is mentioned two times in INSTALL. Once for building "GRUB's graphical terminal (gfxterm)", once as "Platform-agnostic tools and data". There is also a section "Debian named packages required mostly for the full suite of filesystem testing" which could host the name of the needed package. Although it is not about filesystems the mentioning could be justified by the word "mostly" in the headline. Or it could be mentioned as own point one level higher under "Prerequisites for make-check". But actually the lack of unifont did not hamper building GRUB from git. So i would expect that this tolerance is somewhat reflected by the tests. How about putting in tests/grub_func_test.in a test for the existence of /boot/grub/fonts/unicode.pf2 with an informative error message if it is missing ? Since we know that xorriso under grub-mkrescue will cause early failure if the file is missing, there is no use in running grub-shell with the demand for unicode.pf2. I see that grub_func_test.in already has provisions to declare "Functional test failure". Maybe the new code path could join that. Or it could be declared as skipped. (I still have to find out how to cause a SKIP message in the test log.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: make check: grub-mkrescue fails missing unicode.pf2. Leftovers in /tmp
Hi, Glenn Washburn wrote: > You have the package xfonts-unifont installed correct? Urm, no. (I have no desktop running. Just fvwm.) Is the need for this package documented somewhere ? Well, now it is installed. I do $ ./configure ... GRUB2 will be compiled with following components: ... With unifont from /usr/share/fonts/X11/misc/unifont.pcf.gz ... $ make ... $ make check The complaints about the xorriso run do not appear any more. Now grub_func_test fails with other log messages: qemu-system-i386: -fw_cfg name=etc/sercon-port,string=0: warning: externally provided fw_cfg item names should be prefixed with "opt/" Functional test failure: shift_test: shift_test: PASS ... cmdline_cat_test: tests/video_checksum.c:checksum:615: assert failed: 0 Checksum cmdline_cat_2560x1440xrgba:44 failed: 0x62031fea vs 0x8071678a ... about 300 lines of this kind ... tests/video_checksum.c:checksum:615: assert failed: 0 Checksum cmdline_cat_640x480xi16:1 failed: 0x46514dfc vs 0xc69be699 cmdline_cat_test: FAIL ... gfxterm_menu: tests/video_checksum.c:checksum:615: assert failed: 0 Checksum gfxterm_high_2560x1440xrgba:18 failed: 0xc4dbc501 vs 0xfc345163 ... about 1400 lines ... tests/video_checksum.c:checksum:615: assert failed: 0 Checksum gfxterm_menu_640x480xi16:1 failed: 0x3bc388a0 vs 0xd9f04953 gfxterm_menu: FAIL ... more passed tests ... TEST FAILURE FAIL grub_func_test (exit status: 1) Those messages are outside my range of knowledge. If i shall try to tackle them, i'd need exact instructions. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: make check: grub-mkrescue fails missing unicode.pf2. Leftovers in /tmp
Hi,
i have to correct a confused statement of mine:
> I suspect that ${files} comes from the "--files=" argument that is given
> to ./grub-shell by tests/util/grub-shell.in.
The "--files=" argument is given by tests/grub_func_test.in.
Meanwhile i verified that this argument is the origin of the wish for
file unicode.pf2 by changing it from
--files="/boot/grub/fonts/unicode.pf2"="@builddir@/"unicode.pf2
to
--files="/x=/home/thomas/x
/boot/grub/fonts/unicode.pf2"="@builddir@/"unicode.pf2
This yields again the name of my existing /home/thomas/x before the
xorriso error message about unicode.pf2:
Added to ISO image: file '/x'='/home/thomas/x'
xorriso : FAILURE : Cannot determine attributes of source file
'/home/thomas/projekte/grub_dir/grub/unicode.pf2' : No such file or directory
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: make check: grub-mkrescue fails missing unicode.pf2. Leftovers in /tmp
Hi,
Vladimir Serbinenko wrote:
> Look at the end of configure message. Is free type enabled? Was unifont
> found?
$ ./configure
...
checking whether -Wtrampolines work... yes
checking for freetype2... yes
checking ft2build.h usability... yes
checking ft2build.h presence... yes
checking for ft2build.h... yes
checking whether byte ordering is bigendian... no
checking for freetype2... yes
checking ft2build.h usability... yes
checking ft2build.h presence... yes
checking for ft2build.h... yes
checking for fuse3... no
...
***
GRUB2 will be compiled with following components:
Platform: i386-pc
With devmapper support: No (need libdevmapper header)
With memory debugging: No
With disk cache statistics: No
With boot time statistics: No
efiemu runtime: Yes
grub-mkfont: Yes
grub-mount: No (need fuse or fuse3 libraries)
starfield theme: No (No DejaVu found)
With libzfs support: No (need zfs library)
Build-time grub-mkfont: No (no fonts)
Without unifont (no build-time grub-mkfont)
With liblzma from -llzma (support for XZ-compressed mips images)
With stack smashing protector: No
***
So i would say "freetype2" yes, "unifont" no.
(The system is Debian 11 amd64.)
The test log says:
FAIL grub_func_test (exit status: 1)
In ./grub_func_test i see
# Increase memory as some of tests are high-resolution and need a lot of
memory.
out=`echo all_functional_test | ./grub-shell --timeout=3600
--files="/boot/grub/fonts/unicode.pf2"="./"unicode.pf2 --qemu-opts="-m $mem"`
I believe to see the grub-mkrescue invocation in tests/util/grub-shell.in
exec_show_error "${builddir}/grub-mkrescue" "--output=${isofile}" \
"--override-directory=${builddir}/grub-core" \
--rom-directory="${rom_directory}" \
--locale-directory="${srcdir}/po" \
--themes-directory="${srcdir}/themes" \
$mkimage_extra_arg ${mkrescue_args} \
"/boot/grub/grub.cfg=${cfgfile}" "/boot/grub/testcase.cfg=${source}" \
${files} || exit $?
As proof i added a pathspec with an existing filename before ${files}:
-${files} || exit $?
+/x=/home/thomas/x ${files} || exit $?
This file path then appears in the test failure message:
Added to ISO image: file '/x'='/home/thomas/x'
xorriso : FAILURE : Cannot determine attributes of source file
'/home/thomas/projekte/grub_dir/grub/unicode.pf2' : No such file or directory
So i am sure that the xorriso run in grub-shell.in is the one which fails
and that the bad filename comes from ${files}.
I suspect that ${files} comes from the "--files=" argument that is given
to ./grub-shell by tests/util/grub-shell.in.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
make check: grub-mkrescue fails missing unicode.pf2. Leftovers in /tmp
Hi, i encountered some stumblestones after running "make check": - I see in file ./test-suite.log : FAIL: grub_func_test xorriso 1.5.2 : RockRidge filesystem manipulator, libburnia project. ... xorriso : FAILURE : Cannot determine attributes of source file '[...]/unicode.pf2' : No such file or directory ... ./grub-mkrescue: error: `xorriso` invocation failed . FAIL grub_func_test (exit status: 1) Instead of "[...]/" before "unicode.pf2" the log shows the absolute path of the local git repository of GRUB. I do not see a file unicode.pf2 anywhere in the repo tree after "make". I ran "make check" as ordinary user. But it would be strange if the existence of that file depends on superuser powers. - The /tmp directory is full of "grub" directories, like: drwx-- 2 thomas thomas 4096 Feb 10 09:54 grub-shell.0DW38c5CQP They are mostly empty, except a few with sub-directory "netdir": /tmp/grub-shell.OS3deKSNYs: total 4 drwxr-xr-x 3 thomas thomas 4096 Feb 10 10:25 netdir and some with more content: /tmp/grub-shell.wi7ZdDgpLf: total 12 -rw-r--r-- 1 thomas thomas 347 Feb 10 10:25 grub.cfg drwxr-xr-x 2 thomas thomas 4096 Feb 10 10:25 rom_directory -rw-r--r-- 1 thomas thomas 20 Feb 10 10:25 testcase.cfg The timestamps of a group of data files in /tmp suggests that they stem from the "make check: run, too: -rw-r--r-- 1 thomas thomas 17408 Feb 10 10:13 tmp.2XL4eFJ59j (No .iso files are to see. I looked into /tmp after realizing that those would not appear in the repo but in /tmp.) - And a noob question: Is there a way to only run the iso9660_test with all surrounding code of "make check" being in effect ? (I looked into Makefile but got stuck when looking for the entrails of target "check-recursive".) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] tests: Add pathological iso9660 filesystem tests
Hi,
On Wed, 8 Feb 2023 23:52:17 -0600 Glenn Washburn
wrote:
> These are not added to grub-fs-tester because they are not generated and
> none of the filesystem tests are run on these isos. The test is to run the
> command "ls /" on the iso, and a failure is determined if the command
> times out, has non-zero return value or has any output.
>
> Signed-off-by: Glenn Washburn
> ---
> v2: Remove temp directory containing extracted ISOs
> ---
> tests/iso9660_ce_loop.iso.gz | Bin 0 -> 897 bytes
> tests/iso9660_ce_loop2.iso.gz | Bin 0 -> 904 bytes
> tests/iso9660_test.in | 12 +++-
> 3 files changed, 11 insertions(+), 1 deletion(-)
> create mode 100644 tests/iso9660_ce_loop.iso.gz
> create mode 100644 tests/iso9660_ce_loop2.iso.gz
>
> diff --git a/tests/iso9660_ce_loop.iso.gz b/tests/iso9660_ce_loop.iso.gz
> new file mode 100644
> index
> ..9c53c569b8e5c2441273d49f4c8e13e4b3303c24
> GIT binary patch
> literal 897
> zcmb2|=3ofBusfN7`Rxt={M#uq3<N2(O2EDtVk
> zV!mF;l$o*IlVjfrJNEhM(>tw&wEQkz$h$g?`Kr;{^NWwz@Be?_yzkL zBMZ*vjOI-%PkDY@P!aue*Zzaq4`m;3TYuF0MM=}%&};V&Ru zcYbM}Jd5?~Z?n_>ZnR$cYS&()@00TO-|oL;;k(=OmM{C=iVM zRp+eIuf%xF7tgP@mEZA=`Q1@Zn;v%0TmFwNc1#i6|FFnSBBxq3=X&U$oWoNsY
> z#_wqSvIkrf>qL%wO1MZ*@eTFc#D4Vlsq24hKSos?-|;s4BcIjWg#4*n%M0S}#LK*6
> zd8sv(ZLaGJmh*?A=iX*Ncy8C zc_w=Nw?7?zT(xV}=5vy!;+LKuy&ZR+ecj)N>Ovv@$BvV}UAExUOFfdmV~!x(?Ksxk
> zrK0w?3sn8izx&T{;OocR$I{=Vee?YBJpSODC#ye%Zd);Fuhnh6m|Lc2+~eEk&)ZsW
> z`{Zcdjp)!o
> zZ z_J6@^rIx?#B>5)i{M*p+O6$Mgs?&?vT|@kyyUw*gv+V89HS3;*#`TJ;_n%t4sB_li
> zn`>4*zqn%6;>fF0)Gw}(5fA*lW2X4z6`E={OX8lL&523z6?}TNWZu)yFIMDy$|$R`
> z(3{=vx;N*}nXWxDQ!CB%S7%-cIbIr9vUpqjBd4QfC${Yl+-Wg)+hh&($iEg zu2=lj^TiF$Y|9O9;
> z{>Z9lzF+YF?B9bQs^+9VK7Z(e&Ca*$l^EU!?x^|p?Y})kjQVNo`YdodlHa_%`u03V
> Tj6@S-|DS2eE8$QE1||jom5H3-
>
> literal 0
> HcmV?d1
>
> diff --git a/tests/iso9660_ce_loop2.iso.gz b/tests/iso9660_ce_loop2.iso.gz
> new file mode 100644
> index
> ..f59e676475cfc8376256ba65047451b5ca969e5a
> GIT binary patch
> literal 904
> zcmb2|=3rP?urHZ``Rxt={M#uq3<`+#@LT?l
> zg?-)QBTE*}RZ}c{9Q>yELuJ{_9m)%n)Op{A ziNtYF2^VRR(nz1p>_2y(y8f5<9>;sh?Uh^KFSyM4EiAJ7cly!4-wLcf!dIHS*s@UA
> zhCja~?;X4K>u z=Bjg6=`r)Y=Zof7+sf~F#{BN6r%eyL=Pm!o7CR;g?thx(CXrJunsYt%PtM_`7M6Se
> zGvjwOe%S++4__GcO*ZH-(|EOMMjHR8@~P{8|9;f_VP4_v@ZWq^a})BXZrxrVb|;^I
> zdvnQDuVz1KtAn3ExIKSXw_$VM{>*FgyB}{_HA}l#|K+#ce>Hk9Z<)R1ZpnpS_PvRh
> z_DlPAH81<~V8yJpuhtYF`?~W> zuq^i2d9#?V$(MiIGZd8m+4NCz_q=+cfB)PK?Dq-pJ9kU#=IhIE54FY~PWqGj<7MV=
> z^TQhZMZbkjt$KZ{e(mA!v!4n4+kLQ}``!9gckV|Xox5#8@#`(KKin$(K09USf$93U
> z_K53WPu_DY_wc*brGL_UmVGp;5uLLv?egoy4{v@Hs^?t4q*uOo+09Lx3(C@`?%Vuv
> z62pOdz4dYJ)!p%jS7|-FuX;FflCIpdz|g*Zy{jI5aFi3>>>AfAm$vy?P2AM(q7c7Z
> zliiOd#<_pJSQ8i4oe>juanG9W#~Cp`pH|ex=zq!#d44%(n}*xhnn~%Ua=oW+DoN`u
> zI=o2E_4DFp&p`V#t5Y9W&Fk+fG(08dYT5nj(+a7lcUQbrv=84Xx#?wI%$DVw(}a$i
> ztd?`{KXvk=N&l>uJ+oYOPKCC=-q;m6d6wSpw`r5l&z}A9-B$T}u63rnyC&xpUCJp~
> z_x|69FDEC@Z?=D|^7ifb{rl`C;(9mde2SEqqq_ZXD9eL-5xx0)|NiG^2tWDh-27Xh
> c^m5?%o16ExrDG
> literal 0
> HcmV?d1
>
> diff --git a/tests/iso9660_test.in b/tests/iso9660_test.in
> index ed0a5bf8d4..0ec28bd066 100644
> --- a/tests/iso9660_test.in
> +++ b/tests/iso9660_test.in
> @@ -12,4 +12,14 @@ fi
> "@builddir@/grub-fs-tester" rockridge_joliet
> "@builddir@/grub-fs-tester" joliet_1999
> "@builddir@/grub-fs-tester" rockridge_1999
> -"@builddir@/grub-fs-tester" rockridge_joliet_1999
> \ No newline at end of file
> +"@builddir@/grub-fs-tester" rockridge_joliet_1999
> +
> +for fs in iso9660_ce_loop iso9660_ce_loop2; do
> + tempdir=`mktemp -d "${TMPDIR:-/tmp}/${0##*/}.$(date
> '+%Y%m%d%H%M%S%N').${fs}.XXX"` ||
> +{ echo "Failed to make temporary directory"; exit 99; }
> + gunzip <"$srcdir"/tests/${fs}.iso.gz >"${tempdir}/${fs}.iso"
> + output=$(LC_ALL=C timeout -s KILL "60" \
> +"@builddir@/grub-fstest" "${tempdir}/${fs}.iso" ls / ) || ret=$?
> + rm -rf "$tempdir"
> + test "${ret:-0}" -eq 0 -a -z "$output"
> +done
> --
> 2.34.1
Tested-by: Thomas Schmitt
I tested with the current state of
https://git.savannah.gnu.org/git/grub.git
by
make check
with the expected results.
With Lidong Chen's patch 5 v3 still in effect:
PASS: iso9660_test
With the loop breaker of that patch disabled:
FAIL: iso9660_test
after about 60 seconds.
With loop breaker enabled again, but the special handling of CE at the
start of a contin
Re: [PATCH] tests: Add pathological iso9660 filesystem tests
Hi,
Glenn Washburn wrote:
> --- a/tests/iso9660_test.in
> +++ b/tests/iso9660_test.in
> @@ -12,4 +12,13 @@ fi
> "@builddir@/grub-fs-tester" rockridge_joliet
> "@builddir@/grub-fs-tester" joliet_1999
> "@builddir@/grub-fs-tester" rockridge_1999
> -"@builddir@/grub-fs-tester" rockridge_joliet_1999
> \ No newline at end of file
> +"@builddir@/grub-fs-tester" rockridge_joliet_1999
> +
> +for fs in iso9660_ce_loop iso9660_ce_loop2; do
> + tempdir=`mktemp -d "${TMPDIR:-/tmp}/${0##*/}.$(date
> '+%Y%m%d%H%M%S%N').${fs}.XXX"` ||
> +{ echo "Failed to make temporary directory"; exit 99; }
> + gunzip <"$srcdir"/tests/${fs}.iso.gz >"${tempdir}/${fs}.iso"
> + output=$(LC_ALL=C timeout -s KILL "60" \
> +"@builddir@/grub-fstest" "${tempdir}/${fs}.iso" ls / )
> + test "$?" -eq 0 -a -z "$output"
> +done
Shouldn't the decompressed files and the temporary directory be removed
afterwards ?
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read
Hi, Glenn Washburn wrote: > Why does only one suffice? It > sounds like they test different code paths. Is it possible that there > is a future code regression such that one iso succeeds and the other > fails? They follow different code paths before hunk 4 of patch 5 fixes the bug that CE and ST at the start of a continuation area are ignored: @@ -331,6 +340,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off, return err; entry = (struct grub_iso9660_susp_entry *) sua; + /* + * The hook function will not process CE or ST. + * Advancing to the next entry would skip them. + */ + if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0 + || grub_strncmp ((char *) entry->sig, "ST", 2) == 0) + continue; } if (hook (entry, hook_arg)) After this change, the first three hunks of patch 5 prevent that the now common code path is an endless loop. So a behavioral difference of ce_loop.iso and ce_loop2.iso is to expect only if above patch hunk #4 gets reverted. > Ok, so there should be no output on success then for both ce_loop and >ce_loop2, correct? (for "grub-fstest ls /" ) Yes. Actually i would have expected an error message to be emitted. But somehow grub-fstest does not show the text from: + return grub_error (GRUB_ERR_BAD_FS, +"suspecting endless CE loop"); Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read
Hi,
i wrote:
> > So it would be better to add one or two canned images:
> > 897 bytes of http://scdbackup.webframe.org/ce_loop.iso.gz
> > 904 bytes of http://scdbackup.webframe.org/ce_loop2.iso.gz
Glenn Washburn wrote:
> These are small, I'm good with adding these images to the repository.
After the fixes about CE, one of them would suffice.
ce_loop.iso is the easier hack and thus more probable to be created
by some hoaxer. It does not loop endlessly with GRUB before the fixes.
ce_loop2.iso loops endlessly already with the older GRUB versions.
> > I would want to run
> > gunzip ce_loop.iso
> > run_grubfstest ls /
> > in the neighborhood of the xorriso runs and then bail out immediately.
> Why do you want to bail out at this point? I think what the case
> statements should look like is:
>
>x"iso9660_ce_loop")
>gunzip <"@srcdir@"/tests/ce_loop.iso.gz
> >"${FSIMAGEP}0.img" ;;
The hoax ISOs do not contain the files which the further tests in
grub-fs-tester obviously expect. Like:
if [ x$NOHARDLINK != xy ]; then
if run_grubfstest cmp "$GRUBDIR/$BASEHARD"
"$MNTPOINTRO/$OSDIR/$BASEFILE" ; then
So i thought that a single test should be done immediately after
unpacking the iso.gz and then all other tests should be skipped.
> You were talking about the test endlessly looping above, since these
> are native tests we'd put a timeout in the wrapper script that calls
> grub-fs-tester. That would look like adding the following line to the end of
> tests/iso9660_test.in:
> timeout -s KILL "3600" "@builddir@/grub-fs-tester" iso9660_ce_loop
I was not aware of the timeout command.
> A better timeout value could probably be selected. It should be as short as
> possible, but also accounting for the fact that the tests may be run on
> slower machines or in virtual machines.
I think 60 seconds of timeout should suffice for 10 loop cycles in C
with a function call and repeated reading of the same disk block.
If this lasts longer than a minute, then we should reduce the limit of
100,000 loop hops.
After applying Lidong Chen's patches i get on a 4 GHz Xeon with nvme disk:
$ time ./grub-fstest /u/test/ce_loop.iso ls /
real0m0.086s
...
$ time ./grub-fstest /u/test/ce_loop2.iso ls /
real0m0.088s
...
Regrettably there is no error message to see.
But the fact that grub-fstest neither loops endlessly nor shows a file
named "x" indicates that our intention is fulfilled by the patches.
> I see some other modifications that I'd like to
> make to grub-fs-tester, so I could make the changes to add this as well with
> your guidance.
I would be happy if you create the new test.
The only guidance i can offer are the download addresses and the SHA256s:
http://scdbackup.webframe.org/ce_loop.iso.gz
d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
http://scdbackup.webframe.org/ce_loop2.iso.gz
a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c
If only one of them shall be tested, then i propose ce_loop2.iso .
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 0/5] fs/iso9660: Fix out-of-bounds read
Hi, Daniel Kiper wrote: > Thomas, it would be nice if you could add the broken ISOs images which you > used for tests to the tests in the GRUB. If you do that please CC Glenn. Is it wise to have a test which will loop endlessly in case of failure ? Is there a way to let a test time out ? Whatever: After poking in my memory and GRUB's tests directory i came to tests/util/grub-fs-tester.in which produces its ISOs as needed. It could be appropriate to create one or both CE loop ISOs there. But this might become a problem in the future, because the post-production hacks depend on correct byte addresses in the ISO image. So it would be better to add one or two canned images: 897 bytes of http://scdbackup.webframe.org/ce_loop.iso.gz 904 bytes of http://scdbackup.webframe.org/ce_loop2.iso.gz Next problem is that these images do not go well with the other tests in grub-fs-tester.in. I would want to run gunzip ce_loop.iso run_grubfstest ls / in the neighborhood of the xorriso runs and then bail out immediately. But i don't yet fully understand what the for-loops around the xorriso runs mean: for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do ... for BLKSIZE in $blksizes; do ... for NDEVICES in $(range "$MINDEVICES" "$MAXDEVICES" 1); do ... x"ziso9660") FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00); xorriso ... So how to bail out properly at this point after e.g. x"iso9660_ce_loop") gunzip ce_loop.iso run_grubfstest ls / ? And why do the ls tests in grub-fs-tester.in look like run_grubfstest ls -- -la which i cannot decipher by help of the options[]-list in grub-fstest.c ? I CC Glenn Washburn already now, in the hope that he can point me to examples or states that these ISOs should not become part of the tests. (Crossing fingers for the latter case ... ;-) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 5/5] fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi,
On Fri, 20 Jan 2023 19:39:42 + Lidong Chen wrote:
> If processing of a SUSP CE entry leads to a continuation area which
> begins by entry CE or ST, then these entries were skipped without
> interpretation. In case of CE this would lead to premature end of
> processing the SUSP entries of the file. In case of ST this could
> cause following non-SUSP bytes to be interpreted as SUSP entries.
>
> Signed-off-by: Thomas Schmitt
> Tested-by: Lidong Chen
> ---
> grub-core/fs/iso9660.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index ca45b3424..3ddb06ed4 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -50,6 +50,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
> #define GRUB_ISO9660_VOLDESC_END 255
>
> #define GRUB_ISO9660_SUSP_HEADER_SZ 4
> +#define GRUB_ISO9660_MAX_CE_HOPS 10
>
> /* The head of a volume descriptor. */
> struct grub_iso9660_voldesc
> @@ -270,6 +271,7 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
>char *sua;
>struct grub_iso9660_susp_entry *entry;
>grub_err_t err;
> + int ce_counter = 0;
>
>if (sua_size <= 0)
> return GRUB_ERR_NONE;
> @@ -304,6 +306,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> struct grub_iso9660_susp_ce *ce;
> grub_disk_addr_t ce_block;
>
> + if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
> + {
> + grub_free (sua);
> + return grub_error (GRUB_ERR_BAD_FS,
> + "suspecting endless CE loop");
> + }
> +
> ce = (struct grub_iso9660_susp_ce *) entry;
> sua_size = grub_le_to_cpu32 (ce->len);
> off = grub_le_to_cpu32 (ce->off);
> @@ -331,6 +340,13 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> return err;
>
> entry = (struct grub_iso9660_susp_entry *) sua;
> + /*
> +* The hook function will not process CE or ST.
> +* Advancing to the next entry would skip them.
> +*/
> + if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
> + || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> + continue;
> }
>
>if (hook (entry, hook_arg))
> --
> 2.35.1
Reviewed-by: Thomas Schmitt
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/5] fs/iso9660: Fix out-of-bounds read
Hi, Lidong Chen wrote: > I ran grub-fastest with both ce_loop ISO files. The endless loops were > detected and Grub exited accordingly. Good. > I didn't know where the grub error message > were stored in case of grub-fastest. So you don't see an error message ? I had the same problem a while ago, when i tried to check that my thoughts about the loop end condition in grub_iso9660_susp_iterate() are correct. (This is now covered by your patch 2.) > But, I traced with gdb, and saw the code reported the error. It's on my todo list to learn how to prepare grub-fstest for working with gdb. Currently gdb says "No debugging symbols found in ./grub-fstest". > If the diff looks good, I will send the v3 patches set. I have no objections. If patches 1 to 4 are included in v3, please tell whether they have changed towards v2. (I see no reason why they should change. But if they do, i'll have to compare them with the earlier versions.) - I still riddle about how the error message can become visible to the user. I don't get ideas for that from https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Error-Handling I wonder what is supposed to happen to the "textual message" component of a grub_error() call. Under which conditions will it be displayed ? And where ? Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/5] fs/iso9660: Fix out-of-bounds read
Hi, i wrote: > > libisofs and xorriso are in the process of getting an adjustable curb to > > prevent the production of ISO filesystems with files which would not show > > up in Linux. I decided for 100,000 hops as hard limit but set the default > > to 31. Lidong Chen wrote: > I am not sure I understand the hard limit vs the default in terms of > checking the number of hops. xorriso produces and reads ISO 9660 filesystems. At production time, the limit will be adjustable. The default will be 31 to prevent files which don't show up when the filesystem is mounted by Linux. The maximum adjustable limit at production time will be 100,000. At read time, e.g. for extracting files or for adding another session to the ISO, the limit will be 100,000. > Since the limit of CE hops has been decided, I meanwhile deem the proposed 1 million allowed hops quite high. An opinion by experienced GRUB developers would be helpful. > if the previous proposed fix still stands, I can create a patch for it. The fix by hop counting is the right thing to do. > The tough part for me is the testing. You can use for testing http://scdbackup.webframe.org/ce_loop.iso.gz SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2 and http://scdbackup.webframe.org/ce_loop2.iso.gz SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c ce_loop.iso currently causes no endless loop in grub-fstest, because the CE entry at the start of the (bad) continuation area is ignored, against the prescriptions of SUSP. It will cause an endless loop after patch 5/5 is applied and the self-pointing CE entry is not ignored any more by mistake. ./grub-fstest ce_loop.iso ls / ce_loop2.iso already now causes an endless loop with ./grub-fstest ce_loop2.iso ls / Both endless loops should be detected and cause a GRUB error when the CE hop counter and loop breaker is in effect. (I can meanwhile provide ISOs which have 32+ CE hops without loop, i.e. righteously storing 64+ KiB of data in the chain of SUSP entries of a file. But that's mainly interesting for testing Linux, not for GRUB.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/5] fs/iso9660: Fix out-of-bounds read
Hi, On Wed, 18 Jan 2023 08:23:53 + Lidong Chen wrote: > Thomas also pointed out the issue of the potential endless > loops by CE. Since the sugguested fix requires a bit more > investigation, and as Thomas pointed out that it should be > handled in a separate patch, the fix is not included in this > this v2 patches set. Because I am not an expert, it would > be better that someone else can work on it. For the background > info and the comments, please see this email. The bottom half > of the email addressed the endless loop issue: > > https://www.mail-archive.com/grub-devel@gnu.org/msg35785.html I had a look at Linux fs/isofs/rock.c about the handling of CE loops. It works with a hop counter like my current untested proposal for GRUB, which i made in above mail. But Linux is very restrictive: /* Maximum number of Rock Ridge continuation entries */ #define RR_MAX_CE_ENTRIES 32 ... ret = -EIO; if (++rs->cont_loops >= RR_MAX_CE_ENTRIES) goto out; So probably my proposed limit of a million is just oversized. But i think that 32 would be too low. The Linux limit is bad news for people who want to put larger data blobs into SUSP controlled blocks, because the size of a continuation area is also restricted by Linux to a single block of 2048 bytes. The EIO in case of too much CE entries does not show up in the system log or on the terminal. Just the file is not listed and not accessible: $ wc /mnt/iso/x wc: /mnt/iso/x: No such file or directory I understand from source code and experiments that the actual maximum number of CE hops in Linux is 31 rather than 32. libisofs and xorriso are in the process of getting an adjustable curb to prevent the production of ISO filesystems with files which would not show up in Linux. I decided for 100,000 hops as hard limit but set the default to 31. (SUSP prescribes that entries of unknown type be "ignored and skipped", which Linux normally does fine. Here it could do better.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 5/5] fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi,
On Wed, 18 Jan 2023 08:23:58 + Lidong Chen wrote:
> If processing of a SUSP CE entry leads to a continuation area which
> begins by entry CE or ST, then these entries were skipped without
> interpretation. In case of CE this would lead to premature end of
> processing the SUSP entries of the file. In case of ST this could
> cause following non-SUSP bytes to be interpreted as SUSP entries.
>
> Signed-off-by: Thomas Schmitt scdbac...@gmx.net
> ---
> grub-core/fs/iso9660.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index ca45b3424..c3ed11f04 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -331,6 +331,18 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> return err;
>
> entry = (struct grub_iso9660_susp_entry *) sua;
> + /*
> +* The hook function will not process CE or ST.
> +* Advancing to the next entry would skip them.
> +*/
> + ce = (struct grub_iso9660_susp_ce *) entry;
> + if (ce_block != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
> + || off != grub_le_to_cpu32 (ce->off))
> + continue;
> + /*
> +* Ending up here indicates an endless loop by self reference.
> +* So skip this bad CE.
> +*/
> }
>
>if (hook (entry, hook_arg))
> --
> 2.35.1
This looks like the part of my retracted v2 proposal which was intended to
break the possible endless loop by self-referring CE entries:
Date: Fri, 16 Dec 2022 13:57:04 +0100
Message-Id: <31992389627932343...@scdbackup.webframe.org>
Subject: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of
continuation area
But the problem of CE and ST at the start of a continuation area is not
fixed by the above.
What remains after postponing the loop breaker is (hopefully) addressed by
my proposal v1:
Date: Fri, 16 Dec 2022 10:42:13 +0100
Message-Id: <19021389617225107...@scdbackup.webframe.org>
Subject: Proposal: fs/iso9660: Prevent skipping CE or ST at start of
continuation area
--- grub-core/fs/iso9660.c.lidong_chen_patch_2 2022-12-15 11:46:50.654295924 +0
100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix 2022-12-16 10:05:52.9905
99283 +0100
@@ -336,6 +336,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n
}
entry = (struct grub_iso9660_susp_entry *) sua;
+
+ if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
+ || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+ /* The hook function will not process CE or ST.
+Advancing to the next entry would skip them. */
+ continue;
}
if (hook (entry, hook_arg))
---
Bystanders please note that the correctness of my "continue" depends
on Lidong Chen's patch 2, which changes the "for"-loop to a "while"-loop
with incrementation at the loop's end:
- for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char
*) sua + sua_size - 1 && entry->len > 0;
- entry = (struct grub_iso9660_susp_entry *)
-((char *) entry + entry->len))
+ entry = (struct grub_iso9660_susp_entry *) sua;
+
+ while (entry->len > 0)
{
... loop content ...
+ entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
+
+ if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
+break;
}
In current grub-core/fs/iso9660.c we would have to goto the start of
the loop content, circumventing the incrementation step of "for".
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 4/5] fs/iso9660: Incorrect check for entry boundary
Hi,
On Wed, 18 Jan 2023 08:23:57 + Lidong Chen wrote:
> An SL entry consists of the entry info and the component area.
> The entry info should take up 5 bytes instead of sizeof (*entry).
> The area after the first 5 bytes is the component area. It is
> incorrect to use the sizeof (*entry) to check the entry boundary.
>
> Signed-off-by: Lidong Chen
> Reviewed-by: Thomas Schmitt
> ---
> grub-core/fs/iso9660.c | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index c6d65fc22..ca45b3424 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -663,10 +663,23 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>else if (grub_strncmp ("SL", (char *) entry->sig, 2) == 0)
> {
>unsigned int pos = 1;
> + unsigned int csize;
>
> - /* The symlink is not stored as a POSIX symlink, translate it. */
> - while (pos + sizeof (*entry) < entry->len)
> + /* The symlink is not stored as a POSIX symlink, translate it. */
> + while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ + 1) < entry->len)
> {
> + /*
> +* entry->len is GRUB_ISO9660_SUSP_HEADER_SZ + 1 (the FLAGS)
> +* + length of the "Component Area". The length of a component
> +* record is 2 (pos and pos + 1) plus the "Component Content",
> +* of which starts at pos + 2. entry->data[pos] is the
> +* 'Component Flags'; entry->data[pos + 1] is the length
> +* of the component.
> +*/
> + csize = entry->data[pos + 1] + 2;
> + if (GRUB_ISO9660_SUSP_HEADER_SZ + 1 + csize > entry->len)
> +break;
> +
> /* The current position is the `Component Flag'. */
> switch (entry->data[pos] & 30)
> {
> --
> 2.35.1
Reviewed-by: Thomas Schmitt
Most of my initial objections towards patch 4 were wrong. What remained
is taken into respect now.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 3/5] fs/iso9660: Avoid reading past the entry boundary
Hi,
On Wed, 18 Jan 2023 08:23:56 + Lidong Chen wrote:
> Added a check for the SP entry data boundary before reading it.
>
> Signed-off-by: Lidong Chen
> Reviewed-by: Thomas Schmitt
> ---
> grub-core/fs/iso9660.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 65c8862b6..c6d65fc22 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -409,6 +409,9 @@ set_rockridge (struct grub_iso9660_data *data)
>if (!sua_size)
> return GRUB_ERR_NONE;
>
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +return grub_error (GRUB_ERR_BAD_FS, "invalid rock ridge entry size");
> +
>sua = grub_malloc (sua_size);
>if (! sua)
> return grub_errno;
> @@ -435,8 +438,17 @@ set_rockridge (struct grub_iso9660_data *data)
>rootnode.have_symlink = 0;
>rootnode.dirents[0] = data->voldesc.rootdir;
>
> - /* The 2nd data byte stored how many bytes are skipped every time
> - to get to the SUA (System Usage Area). */
> + /* The size of SP (version 1) is fixed to 7. */
> + if (sua_size < 7 || entry->len < 7)
> + {
> + grub_free (sua);
> + return grub_error (GRUB_ERR_BAD_FS, "corrupted rock ridge entry");
> + }
> +
> + /*
> + * The 2nd data byte stored how many bytes are skipped every time
> + * to get to the SUA (System Usage Area).
> + */
> data->susp_skip = entry->data[2];
>entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
> entry->len);
>
> --
> 2.35.1
Reviewed-by: Thomas Schmitt
My minor objections towards v1 are now addressed.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 2/5] fs/iso9660: Prevent read past the end of system use area
Hi,
On Wed, 18 Jan 2023 08:23:55 + Lidong Chen wrote:
> In the code, the for loop advanced the entry pointer to the
> next entry before checking if the next entry is within the
> system use area boundary. Another issue in the code was that
> there is no check for the size of system use area. For a
> corrupted system, the size of system use area can be less than
> the size of minimum SUSP entry size (4 bytes). These can cause
> buffer overrun. The fixes added the checks to ensure the read is
> valid and within the boundary.
>
> Signed-off-by: Lidong Chen
> ---
> grub-core/fs/iso9660.c | 30 +++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 4f4cd6165..65c8862b6 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
> #define GRUB_ISO9660_VOLDESC_PART3
> #define GRUB_ISO9660_VOLDESC_END 255
>
> +#define GRUB_ISO9660_SUSP_HEADER_SZ 4
> +
> /* The head of a volume descriptor. */
> struct grub_iso9660_voldesc
> {
> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
>if (sua_size <= 0)
> return GRUB_ERR_NONE;
>
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
> +
>sua = grub_malloc (sua_size);
>if (!sua)
> return grub_errno;
> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
>if (err)
> return err;
>
> - for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry <
> (char *) sua + sua_size - 1 && entry->len > 0;
> - entry = (struct grub_iso9660_susp_entry *)
> - ((char *) entry + entry->len))
> + entry = (struct grub_iso9660_susp_entry *) sua;
> +
> + while (entry->len > 0)
> {
> + /* Ensure the entry is within System Use Area */
> + if ((char *) entry + entry->len > (sua + sua_size))
> +break;
> +
>/* The last entry. */
>if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> break;
> @@ -300,6 +309,16 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> off = grub_le_to_cpu32 (ce->off);
> ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>
> + if (sua_size <= 0)
> + break;
> +
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> + {
> + grub_free (sua);
> + return grub_error (GRUB_ERR_BAD_FS,
> + "invalid continuation area in CE entry");
> + }
> +
> grub_free (sua);
> sua = grub_malloc (sua_size);
> if (!sua)
> @@ -319,6 +338,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> grub_free (sua);
> return 0;
> }
> +
> + entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
> entry->len);
> +
> + if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
> +break;
> }
>
>grub_free (sua);
> --
> 2.35.1
Reviewed-by: Thomas Schmitt
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 1/5] fs/iso9660: Add check to prevent infinite loop
Hi,
On Wed, 18 Jan 2023 08:23:54 + Lidong Chen wrote:
> There is no check for the end of block when reading
> directory extents. It resulted in read_node() always
> read from the same offset in the while loop, thus
> caused infinite loop. The fix added a check for the
> end of the block and ensure the read is within directory
> boundary.
>
> Signed-off-by: Lidong Chen
> Reviewed-by: Thomas Schmitt
> ---
> grub-core/fs/iso9660.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 91817ec1f..4f4cd6165 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
> while (dirent.flags & FLAG_MORE_EXTENTS)
> {
> offset += dirent.len;
> +
> + /* offset should within the dir's len. */
> + if (offset > len)
> + {
> + if (ctx.filename_alloc)
> + grub_free (ctx.filename);
> + return 0;
> + }
> +
> if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
> {
> if (ctx.filename_alloc)
> @@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
> grub_free (node);
> return 0;
> }
> +
> + /*
> + * It is either the end of block or zero-padded sector,
> + * skip to the next block.
> + */
> + if (!dirent.len)
> + {
> + offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
> + dirent.flags |= FLAG_MORE_EXTENTS;
> + continue;
> + }
> +
> if (node->have_dirents >= node->alloc_dirents)
> {
> struct grub_fshelp_node *new_node;
> --
> 2.35.1
Reviewed-by: Thomas Schmitt
(I'm not sure whether is appropriate to add another Reviewed-by after it
was already given and only a minor cosmetic change was made to the patch.
If this is not ok, then please give me a note.)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi, Lidong Chen wrote: > To test it, I am thinking to add the ISO entry in 40_custom script, then > select > the ISO from Grub menu. Is it the right way to test it? Or, is there a better > way > to it? I have to leave the answer to the experienced GRUB developers. Testing is my weak spot with GRUB. About 6 weeks ago i tried to demonstrate a risk for memory fault and grub-fstest simply did not want to fail. This reminds me that i should have tested my ISOs with grub-fstest before posting them. To my luck the program as pulled 6 weeks ago behaves like i predicted: $ ./grub-fstest ce_loop.iso ls / x $ ./grub-fstest ce_loop2.iso ls / ^C $ I waited about half a minute (on a 4 GHz Xeon) for the second run to end. Then i aborted it by Ctrl+C. As i am at it, i tried with Linux kernel "5.10.0-13-amd64 #1 SMP Debian 5.10.106-1 (2022-03-17)": # mount ce_loop.iso /mnt/iso mount: /mnt/iso: WARNING: source write-protected, mounted read-only. # ls -l /mnt/iso total 0 # No file /x but also no special messages in dmesg. Only: [ ...] loop: module loaded [ ...] ISO 9660 Extensions: RRIP_1991A Same behavior with the ISO which drives grub-fstest into the endless loop: # umount /mnt/iso # mount ce_loop2.iso /mnt/iso mount: /mnt/iso: WARNING: source write-protected, mounted read-only. # ls -l /mnt/iso total 0 # So Linux seems to be safe against this hack. (I will have a look into the source in order to learn how this situation gets handled.) > Thanks a lot for the detail instruction! It is very helpful for the test as > well as for my learning. That's the topic where i can be of use. Don't hesitate to ask for explanations, pointers to the specs, or nastily manipulated ISOs. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi,
i created another bad ISO which i expect to lead to an endless loop in
existing GRUB (i.e. before applying the proposed change).
Both ISOs can be downloaded as gzip-compressed files now:
http://scdbackup.webframe.org/ce_loop.iso.gz
SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
http://scdbackup.webframe.org/ce_loop2.iso.gz
SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c
They are smaller than 1 KB and expand to 128 KiB, each.
(Please do not load these ISOs into existing xorriso programs by commands
like -indev. ce_loop.iso leads to SIGSEGV. ce_loop2.iso leads to an
endless loop. The libisofs code fix is only in git for now.)
Test proposals for GRUB:
If reading ce_loop.iso by GRUB leads to an error message, then my proposed
change is in effect and protected against endless loops.
Original GRUB is supposed to just ignore that situation, because it skips
over the CE entry by mistake.
If reading ce_loop2.iso by GRUB leads to an error message, then the
proposed safety precaution against endless loops is in effect.
I expect unpatched GRUB to loop endlessly with this ISO.
About the production of ce_loop2.iso:
The CE entry of its file /x points to a dummy SUSP entry XY of length 8
which sits directly before the CE entry. So this dummy entry is the
first to be read by GRUB from the pseudo-contination area, gets processed
by the hook() function (with no side effects), and is then followed by
the CE entry.
Because of the existence of the XY entry, i expect the CE entry not to
be skipped by existing GRUB.
# Production begins by the bad ISO of January 9 where the CE entry
# points to itself
cp ce_loop.iso ce_loop2.iso
# Cut out a copy of the bad CE entry
dd if=ce_loop2.iso bs=1 skip=102734 count=28 of=ce_entry
# After the CE entry is plenty of unused space in the same block.
# The length of the directory entry of /x plus 8 will not exceed 255.
# So put the copy back with an offset of 8 bytes.
dd if=ce_entry bs=1 seek=102742 conv=notrunc of=ce_loop2.iso
# Rename the old CE entry head to XY
echo "XY" | dd bs=1 seek=102734 count=2 conv=notrunc of=ce_loop2.iso
# Give it the length of 8
echo $'\x08' | dd bs=1 seek=102736 count=1 conv=notrunc of=ce_loop2.iso
# Set in the new CE entry the continuation area length to 8 + 28 = 36
echo $'\x24' | dd bs=1 seek=102762 count=1 conv=notrunc of=ce_loop2.iso
echo $'\x24' | dd bs=1 seek=102769 count=1 conv=notrunc of=ce_loop2.iso
# Change the length of the directory record from 134 to 142
echo $'\x8e' | dd bs=1 seek=102628 count=1 conv=notrunc of=ce_loop2.iso
The resulting bytes of the whole directory record of /x are then:
000190e0 :.. .. .. .. 8e 00 37 00 00 00 00 00 00 37 02 00
. . . . 7 7
102624 : ... ... ... ... 142 0 55 0 0 0 0 0 0 55 2 0
000190f0 :00 00 00 00 00 02 7b 01 09 08 08 1c 00 00 00 00
{
102640 : 0 0 0 0 0 2 123 1 9 8 8 28 0 0 0 0
00019100 :01 00 00 01 04 58 2e 3b 31 00 50 58 24 01 a4 81
X . ; 1 P X $
102656 : 1 0 0 1 4 88 46 59 49 0 80 88 36 1 164 129
00019110 :00 00 00 00 81 a4 01 00 00 00 00 00 00 01 e8 03
102672 : 0 0 0 0 129 164 1 0 0 0 0 0 0 1 232 3
00019120 :00 00 00 00 03 e8 e8 03 00 00 00 00 03 e8 54 46
T F
102688 : 0 0 0 0 3 232 232 3 0 0 0 0 3 232 84 70
00019130 :1a 01 0e 7b 01 09 08 08 1c 00 7b 01 09 08 08 2f
{ { /
102704 :26 1 14 123 1 9 8 8 28 0 123 1 9 8 8 47
00019140 :00 7b 01 09 08 08 1c 00 4e 4d 06 01 00 78 58 59
{ N M x X Y
102720 : 0 123 1 9 8 8 28 0 78 77 6 1 0 120 88 89
00019150 :08 01 32 00 00 00 43 45 1c 01 32 00 00 00 00 00
2 C E 2
102736 : 8 1 50 0 0 0 67 69 28 1 50 0 0 0 0 0
00019160 :00 32 4e 01 00 00 00 00 01 4e 24 00 00 00 00 00
2 N N $
102752 : 0 50 78 1 0 0 0 0 1 78 36 0 0 0 0 0
00019170 :00 24 .. .. .. .. .. .. .. .. .. .. .. .. .. ..
$ . . . . . . . . . . . . . .
102768 : 0 36 ... ... ... ... ... ... ... ... ... ... ... ... ... .
Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi,
Lidong Chen wrote:
> Thanks for the clarification. I created a new patch for the fix and added
> you as the “Signed-off-by”.
> My question is how to test it.
I just sent libisofs into an endless recursion loop. Grrr.
For this i created a small ISO with a file which surely needs a CE, changed
its fields so that it points to itself, and attempted to load it by xorriso.
(At least it ends by SIGSEGV on an exhausted stack and does not cycle
endlessly.)
--
ISO creation:
# A dummy file as payload
echo x >x
# 250 fattr characters to surely exceed the size of a directory entry
long_string=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
# Create ISO with the payload file and attached fattr named user.dummy
test -e ce_loop.iso && rm ce_loop.iso
xorriso -outdev ce_loop.iso \
-xattr on \
-map x /x \
-setfattr user.dummy "$long_string" /x -- \
-padding 0
Next i determined by a hex dumper the location of the only Rock Ridge
entry NM and then the location of the next following CE entry (the first
CE belongs to the root directory):
Byte 102734 decimal = 50 * 2048 + 334 = LBA 0x32 + Offset 0x014e
The size of the continuation area is 270 = 0x010e
The length of CE is 28 = 0x1c
So i patched the own address and size of the CE entry into its fields:
# The block address (little endian and big endian)
echo $'\x32' | dd bs=1 seek=102738 count=1 conv=notrunc of=ce_loop.iso
echo $'\x32' | dd bs=1 seek=102745 count=1 conv=notrunc of=ce_loop.iso
# The byte offset in that block
echo $'\x4e'$'\x01' | dd bs=1 seek=102746 count=2 conv=notrunc of=ce_loop.iso
echo $'\x01'$'\x4e' | dd bs=1 seek=102752 count=2 conv=notrunc of=ce_loop.iso
# The new size of the CE area is the length of the CE entry
echo $'\x1c' | dd bs=1 seek=102754 count=1 conv=notrunc of=ce_loop.iso
echo $'\x1c' | dd bs=1 seek=102761 count=1 conv=notrunc of=ce_loop.iso
# Zeroize the higher valued bytes of the length field
dd if=/dev/zero bs=1 seek=102755 count=6 conv=notrunc of=ce_loop.iso
This really bad CE entry then looks in the hex dump like:
00019140 :00 7b 01 09 08 08 1c 00 4e 4d 06 01 00 78 43 45
{ N M x C E
102720 : 0 123 1 9 8 8 28 0 78 77 6 1 0 120 67 69
00019150 :1c 01 32 00 00 00 00 00 00 32 4e 01 00 00 00 00
2 2 N
102736 :28 1 50 0 0 0 0 0 0 50 78 1 0 0 0 0
00019160 :01 4e 1c 00 00 00 00 00 00 1c 00 00 00 00 00 00
N
102752 : 1 78 28 0 0 0 0 0 0 28 0 0 0 0 0 0
(Note: 00019140 is hex, not octal)
--
I will first try to fix libisofs before i upload this gremlin somewhere.
If you want to try in advance, run above shell commands and check whether
a hex editor shows the expected bytes afterwards. (xorriso is supposed to
be reproducible in respect to sizes and locations. Very old versions might
not fulfill that expectation, though.)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi,
i wrote on Fri, 16 Dec 2022 10:42:13 +0100:
> > If processing of a SUSP CE entry leads to a continuation area which begins
> > by entry CE or ST, then these entries were skipped without interpretation.
> > In case of CE this would lead to premature end of processing the SUSP
> > entries of the file.
> > In case of ST this could cause following non-SUSP bytes to be interpreted
> > as SUSP entries.
> > [...]
> > }
> >
> > entry = (struct grub_iso9660_susp_entry *) sua;
> > +
> > + if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
> > + || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> > + /* The hook function will not process CE or ST.
> > +Advancing to the next entry would skip them. */
> > + continue;
> > }
> >
> >if (hook (entry, hook_arg))
I wrote on Fri, 16 Dec 2022 13:57:04 +0100:
> > i realize that my previous proposal opens a possibility for regression
> > with a very bad ISO image.
> > The danger is in an endless loop by a CE entry which points to itself.
> > [...] So i now propose:
> > }
> >
> > entry = (struct grub_iso9660_susp_entry *) sua;
> > +
> > + /* The hook function will not process CE or ST.
> > +Advancing to the next entry would skip them. */
> > + if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
> > + {
> > + ce = (struct grub_iso9660_susp_ce *) entry;
> > + if (ce_block
> > + != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
> > + || off != grub_le_to_cpu32 (ce->off))
> > + continue;
> > + /* Ending up here indicates an endless loop by self reference.
> > +So skip this bad CE as was done before december 2022. */
> > + }
> > + if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> > + break;
> > }
> >
> >if (hook (entry, hook_arg))
Lidong Chen wrote:
> In the original the CE code, ‘off’ and ‘ce_block’ were assigned with
> the values (highlighted below) that the above suggested fix tries to
> check against. So, it looks like it will never end here.
> off = grub_le_to_cpu32 (ce->off);
> ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
This happens before "entry" gets pointed to newly allocated memory which
then gets filled with the data from the continuation area
grub_free (sua);
sua = grub_malloc (sua_size);
if (!sua)
return grub_errno;
/* Load a part of the System Usage Area. */
err = grub_disk_read (node->data->disk, ce_block, off,
sua_size, sua);
if (err)
{
grub_free (sua);
return err;
}
entry = (struct grub_iso9660_susp_entry *) sua;
Afterwards my proposed check shall peek ahead whether the suspicious new
CE at the begin of this continuation area is a simple hoax which points
to itself.
But meanwhile i think that a full fledged test for an endless loop is
more appropriate.
E.g. by a counter which breaks the loop:
--- grub-core/fs/iso9660.c.lidong_chen_patch_2 2022-12-15 11:46:50.654295924
+0100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v32023-01-06
16:31:30.226698552 +0100
@@ -176,6 +176,8 @@ enum
FLAG_MORE_EXTENTS = 0x80
};
+#define GRUB_ISO9660_MAX_CE_HOPS 100
+
static grub_dl_t my_mod;
@@ -270,6 +272,7 @@ grub_iso9660_susp_iterate (grub_fshelp_n
char *sua;
struct grub_iso9660_susp_entry *entry;
grub_err_t err;
+ int ce_counter = 0;
if (sua_size <= 0)
return GRUB_ERR_NONE;
@@ -307,6 +310,13 @@ grub_iso9660_susp_iterate (grub_fshelp_n
struct grub_iso9660_susp_ce *ce;
grub_disk_addr_t ce_block;
+ if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+ {
+ grub_free (sua);
+ return grub_error (GRUB_ERR_BAD_FS,
+"suspecting endless CE loop");
+ }
+
ce = (struct grub_iso9660_susp_ce *) entry;
sua_size = grub_le_to_cpu32 (ce->len);
off = grub_le_to_cpu32 (ce->off);
I don't add a signed-off-by because this is uncompiled and still needs
thought about the maximum number of continuation hops and the reaction
to a detected overflow of that number. Who does that work is the author
of the patch.
(1 million suffices for 2048 million bytes of SUSP data per file.
You could silently bail out if this number is surpassed.)
The check would be a separate patch, accompanied by my proposal v1 which
then would need no own safety check:
@@ -336,6 +346,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n
}
entry = (struct grub_iso9660_susp_entry *) sua;
+
+ if (grub_strncmp ((char *) entry->
Re: [RFC PATCH] kern/dl: Add module version check
Hi, Pete Batard wrote: > > unlike what is the case for UEFI, one can not expect > > to be able to pick all the GRUB core files needed to convert a GRUB > > based ISO bootable media to a GRUB based USB bootable media [...] > > Typically, one of the > > missing files will be a 'core.img' that can work for USB BIOS boot of a > > MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid > > image will not readily provide. Zhang Boyang wrote: > The situation is, for BIOS boot, there is no grub core file in ISO > because it's written to sectors instead of files? Did I understand > correctly? I think it is rather about the fact that GRUB equipped hybrid ISOs boot on legacy BIOS from USB stick via the MBR code (~440 byte) to the El Torito boot image, which is in charge for booting from optical media. In grub-mkrescue ISOs it is called /boot/grub/i386-pc/eltorito.img The MBR code is taken by grub-mkrescue.c from the file "boot_hybrid.img" in source_dirs[GRUB_INSTALL_PLATFORM_I386_PC]. So no core.img is needed and the filesystem module set can be sparse, because the filesystem type is known from which GRUB will read its further software. Pete Batard wrote: > > Rufus usually recommends to write such images in what it calls > > "ISO mode" (shorthand for "ISO extraction mode") with the ISO content > > extracted to a FAT or NTFS file system and with a GRUB core.img > > bootloader Maybe one should not only put a copy of the EFI boot image's /EFI/BOOT tree into grub-mkrescue ISOs but also an outdoor pack with the GRUB equipment that is needed to make bootable a USB stick with FAT or NTFS. (The filesystem modules are included in Pete Batard's proposal for the /EFI/BOOT tree. But the disk MBR image and core.img are not.) Zhang Boyang wrote: > test whether that commit (052e6068be62) breaks Rufus > (e.g. Rufus with latest git GRUB, to boot Debian ISO). Debian ISOs still boot on legacy BIOS via ISOLINUX. Archlinux, Ubuntu, Fedora, and others meanwhile use GRUB in their ISOs for both, legacy BIOS and EFI. I think that in any case an ISO made by grub-mkrescue should be tested. Maybe a distro developer is here who uses it for making a full fledged installation ISO or a live ISO. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary
Hi,
in summary: Your objections to my objections are correct.
On Wed, 14 Dec 2022 18:55:05 + Lidong Chen wrote:
> >> + /* The symlink is not stored as a POSIX symlink, translate it. */
> >> + while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
On Dec 15, 2022, at 10:20 AM, Thomas Schmitt wrote:
> > This loop is iterating over component records, not SUSP entries.
> > Minimum size of a component record is 2.
> >
> > So i think the appropriate condition is:
> >
> > while (pos + 1 < entry->len)
On Mon, 19 Dec 2022 21:00:23 + Lidong Chen wrote:
> My understanding is the entry->len is the size of the System Use Field.
> the while-loop condition is to make sure the component records are
> within the System Use Field boundary.
> “Pos” was initially set to ‘1’ and
> incremented by the size of each component record. So, “Pos” is relative
> to the Component Area, not the System Use Field of “SL”.
> I think the fix should include the 5 bytes:
>
> While (pos + 5 < entry->len)
You are right.
My "+ 1" would be correct if entry->len was the size of the SL entry
payload (FLAGS byte and COMPONENT AREA). But further "+ 4" are needed
because of the SUSP header size of the SL entry.
(One could use "+ GRUB_ISO9660_SUSP_HEADER_SZ" instead of "+ 4" in this
case. But at other places you followed my proposal to refer with plain
numbers to the description in the RRIP specs. So in the sum "+ 5" is
good.)
> > I am undecided whether add_part() should be skipped if
> > (pos + 2 == entry->len)
> Do you mean this within the while-loop?:
>
>If (pos + 2 == entry->len)
> continue
I meant your change of program behavior:
- add_part (ctx, (char *) &entry->data[pos + 2],
- entry->data[pos + 1]);
+ if (entry->data[pos + 1] > 0)
+ {
+ add_part (ctx, (char *) &entry->data[pos + 2],
+ entry->data[pos + 1]);
+ }
I obviously made some inappropriate thought jumps when writing
(pos + 2 == entry->len)
Apologies for being confusing.
(It's not only about the end of the SL entry but about the size of any
component record, regardless of its position in the entry's payload.
Aggravating is that the condition i mentioned is wrong by the missing
"+ 4" for the SUSP header, even if it was about the SL entry's end.)
Whatever, you already decided not to change program behavior by
skipping over empty text components.
> > Open issues:
> > [...]
> > The while-condition of the component loop should neither use sizeof (*entry)
> > nor GRUB_ISO9660_SUSP_HEADER_SZ, but plain 1, because that's the minimum
> > size of an SL component record minus 1.
> > Probably this should be a separate bug-fix patch to be applied before this
> > patch [4/4].
So i now retract this issue.
> > The change of program behavior by ignoring empty link target path components
> > should be mentioned in the commit message.
This issue is now obsolete.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop
Hi,
i wrote:
> > (Are we aware of the file size limit of 32 GiB - 14 KiB - 1 imposed by
> > struct grub_fshelp_node { ... struct grub_iso9660_dir dirents[8]; ... }
> > ? )
Lidong Chen wrote:
> I am not familiar with this file size limit. Do we need to add a check
> somewhere?
Good question. The answer probably disproves my statement because the
struct definition seems not to match exactly its usage:
Assessment happens in grub_iso9660_iterate_dir():
while (dirent.flags & FLAG_MORE_EXTENTS)
{
...
if (node->have_dirents >= node->alloc_dirents)
{
At this point an overflow of currently allocated .dirents[] was detected.
struct grub_fshelp_node *new_node;
grub_size_t sz;
if (grub_mul (node->alloc_dirents, 2, &node->alloc_dirents) ||
grub_sub (node->alloc_dirents, ARRAY_SIZE (node->dirents),
&sz) ||
grub_mul (sz, sizeof (node->dirents[0]), &sz) ||
grub_add (sz, sizeof (struct grub_fshelp_node), &sz))
goto fail_0;
new_node = grub_realloc (node, sz);
I understand the computations in the if-clause as:
- The number of allocated dirents is doubled.
- The new_node size is the size of the new number of .dirents minus 8
.dirent sizes for the eight .dirents which are part of the
grub_fshelp_node definition,
- plus the defined size of the grub_fshelp_node.
The new_node gets allocated with that size, which provides enough space
for the new dirent and many of its potential successors.
So i retract my statement. Data file size seems quite unlimited.
At some point grub_mul() or grub_realloc() will throw an error if the number
of .dirents is too high for grub_size_t or the machine's memory.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi,
i realize that my previous proposal opens a possibility for regression with
a very bad ISO image.
The danger is in an endless loop by a CE entry which points to itself.
The bug which i want to see fixed currently prevents this special pitfall.
(Other endless loops by CE are possible and not prevented. It's much like
with symbolic link loops. In the end only a hard limit on the number of
CE hops would help.)
So i now propose with the same sketch of a commit message, this change
(compiles but was but not tested):
fs/iso9660: Prevent skipping CE or ST at start of continuation area
If processing of a SUSP CE entry leads to a continuation area which begins
by entry CE or ST, then these entries were skipped without interpretation.
In case of CE this would lead to premature end of processing the SUSP entries
of the file.
In case of ST this could cause following non-SUSP bytes to be interpreted
as SUSP entries.
Signed-off-by: Thomas Schmitt
--- grub-core/fs/iso9660.c.lidong_chen_patch_2 2022-12-15 11:46:50.654295924
+0100
+++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix_v22022-12-16
13:54:55.654651173 +0100
@@ -336,6 +336,21 @@ grub_iso9660_susp_iterate (grub_fshelp_n
}
entry = (struct grub_iso9660_susp_entry *) sua;
+
+ /* The hook function will not process CE or ST.
+Advancing to the next entry would skip them. */
+ if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0)
+ {
+ ce = (struct grub_iso9660_susp_ce *) entry;
+ if (ce_block
+ != grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ
+ || off != grub_le_to_cpu32 (ce->off))
+ continue;
+ /* Ending up here indicates an endless loop by self reference.
+So skip this bad CE as was done before december 2022. */
+ }
+ if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+ break;
}
if (hook (entry, hook_arg))
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Proposal: fs/iso9660: Prevent skipping CE or ST at start of continuation area
Hi, i believe that the problem about CE entries sitting at the start of a SUSP continuation area could be quite easily solved after patch [2/4] was applied, which changes the for-loop to a while loop with the entry-advance step at its end. So i propose to Lidong Chen to handle this problem by another patch in the series, which comes after [2/4] (then [2/5]). Please verify that my thoughts about CE (and additionally about ST) are correct and consider the following diff, which compiles but is not tested otherwise. I agree in advance to changing my "Signed-off-by:" to "Suggested-by:" if deemed appropriate. fs/iso9660: Prevent skipping CE or ST at start of continuation area If processing of a SUSP CE entry leads to a continuation area which begins by entry CE or ST, then these entries were skipped without interpretation. In case of CE this would lead to premature end of processing the SUSP entries of the file. In case of ST this could cause following non-SUSP bytes to be interpreted as SUSP entries. Signed-off-by: Thomas Schmitt --- grub-core/fs/iso9660.c.lidong_chen_patch_2 2022-12-15 11:46:50.654295924 +0100 +++ grub-core/fs/iso9660.c.lidong_chen_patch_2_ce_fix 2022-12-16 10:05:52.990599283 +0100 @@ -336,6 +336,12 @@ grub_iso9660_susp_iterate (grub_fshelp_n } entry = (struct grub_iso9660_susp_entry *) sua; + + if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0 + || grub_strncmp ((char *) entry->sig, "ST", 2) == 0) + /* The hook function will not process CE or ST. +Advancing to the next entry would skip them. */ + continue; } if (hook (entry, hook_arg)) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area
Hi, while preparing a proposal how to avoid skipping of CE (and ST) if they are found at the start of a continuation area, i came to a problem of patch [2/4] which i did not see when reviewing it yesterday: > + return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size"); It is not about the size of the CE entry but about the size of the continuation area which the CE entry announces. So i propose as error message "invalid continuation area size in CE entry" Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary
Hi,
On Wed, 14 Dec 2022 18:55:05 + Lidong Chen wrote:
> [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary
s/boudary/boundary/
> An entry consists of the entry info and the component area.
Component area is specific to the RRIP SL entry. So:
s/An entry/An SL entry/
> The entry info should take up 5 bytes instead of sizeof (*entry).
> The area after the first 5 bytes is the component area. The code
> uses the sizeof (*entry) to check the boundary which is incorrect.
> Also, an entry may not have component record. Added a check for
> for the component length before reading the component record.
>
> Signed-off-by: Lidong Chen
> ---
> grub-core/fs/iso9660.c | 23 +++
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 67aa8451c..af432ee82 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -662,10 +662,22 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
>else if (grub_strncmp ("SL", (char *) entry->sig, 2) == 0)
> {
>unsigned int pos = 1;
> + unsigned int csize;
>
> - /* The symlink is not stored as a POSIX symlink, translate it. */
> - while (pos + sizeof (*entry) < entry->len)
> + /* The symlink is not stored as a POSIX symlink, translate it. */
> + while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
This loop is iterating over component records, not SUSP entries.
Minimum size of a component record is 2.
So i think the appropriate condition is:
while (pos + 1 < entry->len)
Component records have no struct representation in GRUB. So no sizeof will
tell the correct value.
C-wise decisive is this line:
add_part (ctx, (char *) &entry->data[pos + 2],
entry->data[pos + 1]);
which lower in this patch changes to
if (entry->data[pos + 1] > 0)
{
add_part (ctx, (char *) &entry->data[pos + 2],
entry->data[pos + 1]);
}
This change will alter the program behavior in respect to empty link
target path components.
The validity of entry->data[pos + 1] is guaranteed by my test proposal.
If entry->data[pos + 1] is 0, then &entry->data[pos + 2] can be an illegal
address, but 0 bytes from that address will be requested to be added.
(A 0-byte will be added by add_part() as separator between link target path
components.)
I am undecided whether add_part() should be skipped if
(pos + 2 == entry->len)
which indicates an empty path component.
If add_part() shall be performed with length 0, then we should skip in it
the call of
grub_memcpy (ctx->symlink + size, part, len2);
in case of (len2 == 0).
> {
> + /*
> +* entry->len is GRUB_ISO9660_SUSP_HEADER_SZ plus the
> +* length of the 'Component Record'. The length of the
With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be
GRUB_ISO9660_SUSP_HEADER_SZ + 1 + length of the Component Area.
The "+ 1" stands for the "FLAGS" byte. (RRIP-1.12, 4.1.3)
It should be 'Component Area' or plural 'Component Records' because
'Component Record' is a single path component.
> +* record is 2 (pos and pos + 1) plus the actual record
> +* starting at pos + 2. pos stores the 'Component Flags',
> +* pos + 1 specifies the length of actual record.
> +*/
This describes a single component record, of which there are as many as
needed to represent the link target path.
entry->data[pos + 1] gives the length of the component, not of the component
record, of which the length is entry->data[pos + 1] + 2.
> + csize = entry->data[pos + 1] + 2;
> + if (csize + GRUB_ISO9660_SUSP_HEADER_SZ > entry->len)
With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be
if (csize + GRUB_ISO9660_SUSP_HEADER_SZ + 1 > entry->len)
> +break;
> +
> /* The current position is the `Component Flag'. */
> switch (entry->data[pos] & 30)
> {
> @@ -681,8 +693,11 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
> return grub_errno;
> }
>
> - add_part (ctx, (char *) &entry->data[pos + 2],
> - entry->data[pos + 1]);
> + if (entry->data[pos + 1] > 0)
> + {
> + add_part (ctx, (char *) &entry->data[pos + 2],
> + entry->data[pos + 1]);
> + }
As written above, this changes program behavior if a link target path
contains an empty component.
> ctx->was_continue = (entry->data[pos] & 1);
> break;
> }
> --
> 2.35.1
>
So no "Reviewed-by:" yet.
Open issues:
The commit message should mention that it is about SL and not about general
SUSP or RRIP entries.
The while-condition of the component loop should neither use sizeof (*entry)
nor GRUB_ISO9660_SUSP_HEADER_SZ, but plain
Re: [PATCH 3/4] fs/iso9660: Avoid reading past the entry boundary
Hi,
On Wed, 14 Dec 2022 18:55:04 + Lidong Chen wrote:
> Added a check for the SP entry data boundary before reading it.
>
> Signed-off-by: Lidong Chen
> ---
> grub-core/fs/iso9660.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 9170fa820..67aa8451c 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -408,6 +408,9 @@ set_rockridge (struct grub_iso9660_data *data)
>if (!sua_size)
> return GRUB_ERR_NONE;
>
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
As mentioned in my review of patch [2/4], GRUB_ISO9660_SUSP_HEADER_SZ should
be defined as 4, rather than as 5. Else entry RE could trigger this error:
> +return grub_error (GRUB_ERR_BAD_FS, "invalid rock ridge entry size");
> +
>sua = grub_malloc (sua_size);
>if (! sua)
> return grub_errno;
> @@ -434,8 +437,17 @@ set_rockridge (struct grub_iso9660_data *data)
>rootnode.have_symlink = 0;
>rootnode.dirents[0] = data->voldesc.rootdir;
>
> - /* The 2nd data byte stored how many bytes are skipped every time
> - to get to the SUA (System Usage Area). */
> + /*
> + * The 2nd data byte stored how many bytes are skipped every time
> + * to get to the SUA (System Usage Area).
> + */
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ + 2 ||
> + entry->len < GRUB_ISO9660_SUSP_HEADER_SZ + 2)
This interprets an SP entry, which is specified to have 7 bytes.
So if GRUB_ISO9660_SUSP_HEADER_SZ gets corrected to 4, then the size demand
will have to be (GRUB_ISO9660_SUSP_HEADER_SZ + 3).
Like with the NM interpretation i would rather prefer a plain "7", maybe with
a comment which says that this is the fixed size of SP (version 1).
> + {
> + grub_free (sua);
> + return grub_error (GRUB_ERR_BAD_FS, "corrupted rock ridge entry");
> + }
> +
> data->susp_skip = entry->data[2];
>entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
> entry->len);
>
> --
> 2.35.1
>
Reviewed-by: Thomas Schmitt
But the expression (GRUB_ISO9660_SUSP_HEADER_SZ + 2) will need correction if
my wish for
#define GRUB_ISO9660_SUSP_HEADER_SZ 4
gets fulfilled. As said, i'd prefer a plain "7".
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area
Hi,
On Wed, 14 Dec 2022 18:55:03 + Lidong Chen wrote:
> In the code, the for loop advanced the entry pointer to the
> next entry before checking if the next entry is within the
> system use area boundary. Another issue in the code was that
> there is no check for the size of system use area. For a
> corrupted system, the size of system use area can be less than
> the size of SUSP entry size (5 bytes).
The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries
are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, because
ST marks the end of the SUSP entry chain. But RE may appear before the end.)
> These can cause buffer
> overrun. The fixes added the checks to ensure the read is
> valid and within the boundary.
>
> Signed-off-by: Lidong Chen
> ---
> grub-core/fs/iso9660.c | 31 +++
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 4f4cd6165..9170fa820 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
> #define GRUB_ISO9660_VOLDESC_PART3
> #define GRUB_ISO9660_VOLDESC_END 255
>
> +#define GRUB_ISO9660_SUSP_HEADER_SZ 5
So i think this should be 4, not 5.
If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not
appropriate. The SUSP header size is surely 4.
> +
> /* The head of a volume descriptor. */
> struct grub_iso9660_voldesc
> {
> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
>if (sua_size <= 0)
> return GRUB_ERR_NONE;
>
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
> +
Here we need 4.
>sua = grub_malloc (sua_size);
>if (!sua)
> return grub_errno;
> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
>if (err)
> return err;
>
> - for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry <
> (char *) sua + sua_size - 1 && entry->len > 0;
> - entry = (struct grub_iso9660_susp_entry *)
> - ((char *) entry + entry->len))
> + entry = (struct grub_iso9660_susp_entry *) sua;
> +
> + while (entry->len > 0)
> {
> + /* Ensure the entry is within System Use Area */
> + if ((char *) entry + entry->len > (sua + sua_size))
> +break;
> +
>/* The last entry. */
>if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> break;
> @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> off = grub_le_to_cpu32 (ce->off);
> ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>
> + if (sua_size <= 0)
> + break;
> +
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
4 would be appropriate here.
> + {
> + grub_free (sua);
> + return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
> + }
> +
> grub_free (sua);
> sua = grub_malloc (sua_size);
> if (!sua)
> @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> grub_free (sua);
> return 0;
> }
> +
> + entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
> entry->len);
This is equivalent to the third statement in the "for" which was
replaced by a while-loop. So far so good.
But i believe to see an old bug. This advancing of entry breaks the
chain of CE if the first entry of the Continuation Area is again a CE.
err = grub_disk_read (node->data->disk, ce_block, off,
sua_size, sua);
...
entry = (struct grub_iso9660_susp_entry *) sua;
}
if (hook (entry, hook_arg))
{
grub_free (sua);
return 0;
}
entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
hook() will not interpret CE (and has no means to advance "entry").
entry = entry + entry->len; will then skip over the entry so that the loop
will not interpret it either.
The SUSP area will be perceived as having ended, although more SUSP entries
are present for the file in question.
I hereby ask for more opinions about this. Maybe i misinterpret the old loop.
Background:
CE points to the block and offset where the chain of SUSP entries goes on.
It is needed if the SUSP entry set exceeds the size limit of 255 bytes for
a directory record.
The byte at (ce->blk * block_size + ce->off) is the first byte of the next
SUSP entry in the chain.
Linux hates SUSP crossing block boundaries. So we ISO producers use further
CE entries to hop over block boundaries.
libisofs can produce a Continuation Area with first and only entry CE,
which then points to a new block with more SUSP payload. This happens if a
continuation block offers not much more than 28 free bytes, so that only
the 28 b
Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop
Hi,
On Wed, 14 Dec 2022 18:55:02 + Lidong Chen wrote:
> There is no check for the end of block When reading
s/When/when/
> directory extents. It resulted in read_node() always
> read from the same offset in the while loop, thus
> caused infinite loop. The fix added a check for the
> end of the block and ensure the read is within directory
> boundary.
>
> Signed-off-by: Lidong Chen
> ---
> grub-core/fs/iso9660.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 91817ec1f..4f4cd6165 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
> while (dirent.flags & FLAG_MORE_EXTENTS)
> {
> offset += dirent.len;
> +
> + /* offset should within the dir's len. */
> + if (offset > len)
> + {
> + if (ctx.filename_alloc)
> + grub_free (ctx.filename);
> + return 0;
> + }
> +
> if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
> {
> if (ctx.filename_alloc)
> @@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
> grub_free (node);
> return 0;
> }
> +
> + /*
> + * It is either the end of block or zero-padded sector,
> + * skip to the next block.
> + */
> + if (!dirent.len)
> + {
> + offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
> + dirent.flags |= FLAG_MORE_EXTENTS;
> + continue;
> + }
> +
> if (node->have_dirents >= node->alloc_dirents)
> {
> struct grub_fshelp_node *new_node;
> --
> 2.35.1
>
Reviewed-by: Thomas Schmitt
The second hunk will become very necessary when more initrds >= 4 GiB will
be around. Then GRUB might more probably encounter directory records of a
large file which are not stored in the same block.
(Are we aware of the file size limit of 32 GiB - 14 KiB - 1 imposed by
struct grub_fshelp_node { ... struct grub_iso9660_dir dirents[8]; ... }
? )
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/4] fs/iso9660: Fix out-of-bounds read
Hi,
i will review the patches hopefully tomorrow.
But in [PATCH 2/4] and [4/4] i stumble over the statement that a SUSP
entry has 5 bytes of size. This is not true. The minimum size is 4.
In SUSP there are "Padding" PD (if its byte LEN_PD is 0) and "Stop" ST,
which have 4 bytes. In RRIP there is "Relocated" RE.
Other SUSP-compliant protocols could define 4-byte entries, too.
I will have to analyze the patches whether the assumption of 5 bytes
minimum can cause real harm.
But i see at least two inappropriate applications of the minimum size:
In [2/4] a RRIP NM entry is processed:
- csize = entry->len - 5;
+ csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
The number 5 is meant to skip over the 4 bytes of SUSP header and the one
byte of "FLAGS" to reach to the "NAME CONTENT" bytes. This is specific to
NM (version 1, to be exacting), not to SUSP in general.
I propose to leave the 5 as is.
In [4/4] a RRIP SL entry is processed:
- /* The symlink is not stored as a POSIX symlink, translate it. */
- while (pos + sizeof (*entry) < entry->len)
+ /* The symlink is not stored as a POSIX symlink, translate it. */
+ while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len)
At least in a quick test in GNU/Linux userspace
struct grub_iso9660_susp_entry {
uint8_t sig[2];
uint8_t len;
uint8_t version;
uint8_t data[0];
};
has sizeof 4, not 5.
So i see the risk that this change alters program behavior in situations
where we don't perceive a problem yet.
It is too late in the evening to analyze what sizeof(*entry) has to do
with reading the component records of SL. The component records are the
components of the link target path with 2 bytes header plus the name
bytes. So a size of 3 is plausible like in .../b/... Even a size of 2
is not totally illegal, as Linux allows paths like ...//
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Possible memory fault in fs/iso9660 (correction)
Hi, i wrote: > > > I will think about creating such an ISO by help of xorriso and dd. Daniel Kiper wrote: > Yeah, that would be perfect... I believe to have created one. But grub-fstest does not produce a memory fault. See my mail Date: Tue, 29 Nov 2022 19:47:22 +0100 Message-Id: <50363882005823...@scdbackup.webframe.org> for the recipe to create that ISO. I riddle: - Would valgrind detect out-of-bounds reading in GRUB code ? (Or does the code under grub-fstest allocate a large memory chunk on which the memory management of GRUB operates ?) - Is there a way to build the involved code for use under gdb ? - How can i insert debug messages into grub-core/fs/iso9660.c ? > > > [more opportunities to let the code derail] > Huh! Could you fix these issues too? I will try. But first i need to master grub-fstest or some other testbed so that i can verify my theoretical considerations. (The "- 1" problem is obvious from C code considerations. But the number to replace the "1" is not so obvious and in general we should not fix what is not broken.) > > > In general: > > > How mistrusting should GRUB be towards the bytes in the filesystem ? > I think as little as possible. Especially if incorrect values may lead > to OOB writes... It is about out-of-bounds reads. But i don't understand well the combination of your two sentences: GRUB shall be credulent, especially if bad writes were involved ? I would think that this is to be avoided most. So please explain the philosopy a bit more verbous for an old programmer or point me to examples. (I would look into the other fs drivers if i would understand filesystems other than ISO 9660.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Possible memory fault in fs/iso9660 (correction)
Hi,
Fengtao wrote:
> I think:
> (char *) entry < (char *) sua + sua_size - 3 && entry->len > 0
> is ok, or:
> (char *) entry <= (char *) sua + sua_size - 4 && entry->len > 0
"4" would be overdone. There are SUSP and RRIP entries of length 4,
which would be ignored if appearing at the end of the SUSP controlled area.
> I am also not familiar with ISO format.
I seem to be the last active programmer on that topic.
As stated on 24 Nov, i see other potential memory faults in the code of
grub-core/fs/iso9660.c if the ISO image contains incorrect SUSP entries.
---
I began to hack an ISO image so that it shows non-SUSP data after the
SUSP data. (See below.)
But now i am having noob problems with grub-fstest.
I pulled the source from git://git.savannah.gnu.org/grub on Debian 11 and
built it as prescribed in INSTALL.
Nevertheless grub-fstest does not show a memory fault with:
valgrind ./grub-fstest /dvdbuffer/grub_test_non_susp.iso ls /
gdb says that the execution enters grub_iso9660_susp_iterate()
Breakpoint 1, 0x5557dde4 in grub_iso9660_susp_iterate ()
but gives no further information, because
(No debugging symbols found in ./grub-fstest)
Next i tried to insert visible messages into grub_iso9660_susp_iterate():
grub_error (GRUB_ERR_BAD_NUMBER, "grub_iso9660_susp_iterate: called");
...
grub_error (GRUB_ERR_BAD_NUMBER, "grub_iso9660_susp_iterate: before loop");
...
grub_error (GRUB_ERR_BAD_NUMBER,
"grub_iso9660_susp_iterate: sua + sua_size - entry = %ld",
(long int) ((char *) sua + sua_size - (char *) entry));
I do not see any of them when running with above arguments.
So how can i make grub-core/fs/iso9660.c debuggable or let it emit visible
messages ?
---
The test ISO is made by these commands in bash:
# Create an ISO with a playground of SUSP data.
echo dummy >dummy_file
test -e grub_test_non_susp.iso && rm grub_test_non_susp.iso
xorriso \
-xattr on \
-outdev grub_test_non_susp.iso \
-map dummy_file /dummy_file \
-setfattr user.x y /dummy_file -- \
-padding 0
# Search for the AL entry of length 0x0c which holds attribute user.x.
# (AL is the entry type of my invention AAIP. It gets ignored by all
# readers except xorriso. So it is a good playground for manipulations.)
# (There is also another AL entry of size 0x10 in the CE area of the root
# directory.)
al=$(grep -a -o -b 'AL'$'\x0c'$'\x01' grub_test_non_susp.iso | \
sed -e 's/:/ /' | awk '{print $1}')
# Replace length field value 0x0c by 0x0a.
# This leaves the last 2 bytes of the AL entry as trailing non-SUSP data
# in the System Use field of the directory entry.
al_length_offset=$(expr $al + 2)
echo $'\x0a' | \
dd of=grub_test_non_susp.iso \
bs=1 count=1 seek="$al_length_offset" conv=notrunc
Inspection by a hex dumper shows that the AL entry indeed announces the
desired (short) length of 10.
---
I also learned by doing and then by reading specs that a padding byte after
the System Use data is present if needed to get an even number of bytes
as size of the directory record.
This could explain the existing "- 1" in GRUB's code.
Above ISO is supposed to show 3 non-SUSP bytes at the end of the System Use
field: 2 from my dd manipulation, 1 as regular padding.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Possible memory fault in fs/iso9660 (correction)
Hi, (Again i Cc t.feng in the hope that the review is not finished yet. :)) Daniel Kiper wrote: > I am not an ISO format expert but your thinking LGTM. So you agree that "3" is really the right number if any remaining bytes fewer than 4 shall be ignored ? (I don't trust myself, although i made an example with finger counting.) > could you send a patch fixing this issue? This should be possible. But how to test ? Normal ISOs made with GNU/Linux will cause (entry == sua + sua_size) at the end of the SUSP data. So provoking the problem and checking whether it is solved will need a hacked ISO. I will think about creating such an ISO by help of xorriso and dd. While exploring the SUSP/RRIP entry types which are interpreted by GRUB, i got to more credulence towards the ISO producer. E.g. in grub-core/fs/iso9660.c line 404 ff. /* The 2nd data byte stored how many bytes are skipped every time to get to the SUA (System Usage Area). */ data->susp_skip = entry->data[2]; This is a memory fault if (sua_size < 7). I see no check between sua = grub_malloc (sua_size); and the read access to entry->data[2]. Another example: grub_iso9660_susp_iterate() calls its parameter hook() without checking that the alleged entry size is within the range of sua_size. The hook() function does not get sua_size to check on its own. In general: How mistrusting should GRUB be towards the bytes in the filesystem ? Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Possible memory fault in fs/iso9660 (correction)
Hi, i wrote: > I think the loop end condition should use 4 rather than 1: > (char *) entry < (char *) sua + sua_size - 4 && entry->len > 0 Urm ... better "3 rather than 1": (char *) entry < (char *) sua + sua_size - 3 && entry->len > 0 The memory fault by entry->len will appear if entry >= sua + sua_size - 2 (Only good i did not submit a patch attempt. Why is that "- 1" present anyways ? Shall it ensure the presence of entry->type ?) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Possible memory fault in fs/iso9660
Hi,
(Cc-ing t.feng in the hope that this issue can become part of the code
review.)
While reviewing "[PATCH 7/9]" by t.feng, i wonder whether there is a bug
in grub_iso9660_susp_iterate() in regard to the end of the SUSP data:
for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char
*) sua + sua_size - 1 && entry->len > 0;
entry = (struct grub_iso9660_susp_entry *)
((char *) entry + entry->len))
{
I think the loop end condition should use 4 rather than 1:
(char *) entry < (char *) sua + sua_size - 4 && entry->len > 0
The entry struct has at least 4 bytes:
struct grub_iso9660_susp_entry
{
grub_uint8_t sig[2];
grub_uint8_t len;
grub_uint8_t version;
grub_uint8_t data[0];
} GRUB_PACKED;
Especially the expression
entry->len > 0
reads beyond the end of the allocated buffer sua if
entry >= sua + sua_size - 3
It does not look as if there are guard rails installed yet:
The size of the allocated space (parameter sua_size) is determined by the
callers of grub_iso9660_susp_iterate from struct grub_iso9660_dir objects,
which obviously represent ISO 9660 directory entry bytes as read from the
filesystem.
---
So the producer of the ISO filesystem can create in GRUB the impression
of trailing bytes which form no valid SUSP entry. This may even happen in
good faith because a ISO 9660 System Use field may hold data which are not
under control of the SUSP protocol.
There is a "ST" entry specified which explcitely ends the range of SUSP.
But the SUSP specs, both 1.10 and 1.12, mention that a System Use field
or a Continuation area may end without ST entry with up to 3 remaining
bytes which shall be ignored. From SUSP 1.12:
If the remaining allocated space following the last recorded System Use
Entry in a System Use field or Continuation Area is less than four bytes
long, it cannot contain a System Use Entry and shall be ignored.
Otherwise an "ST" System Use Entry (see section 5.4) shall be the last
System Use Entry recorded in a System Use field or Continuation Area.
Neither mkisofs/genisominage nor libisofs write any ST entries. At least
for libisofs i can state that there is no trailing non-SUSP data announced
by the size of the directory entry or the Continuation Area.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 7/9] fs/iso9660: Fix memory leak in grub_iso9660_susp_iterate
Hi, on Sat, 19 Nov 2022 18:39:44 +0800, t.feng via Grub-devel wrote: > Fix memory leak in grub_iso9660_susp_iterate. > > Fixes: 99373ce47(iso9660.c: Remove nested functions.) > > Signed-off-by: "t.feng" Reviewed-by: Thomas Schmitt Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/3] grub-mkrescue: Preserve a copy of the EFI bootloaders on the ISO9660 file system
Hi, On Mon, 6 Jun 2022 17:50:32 +0100, Pete Batard wrote: > To enable file system transposition support for UEFI, we also must ensure that > there exists a copy of the EFI bootloaders, that are currently embedded in the > efi.img for xorriso, at their expected UEFI location on the ISO9660 file > system. > > This is accomplished by removing the use of a temporary directory to create > the > efi/ content, to instead place it at the root of the ISO9660 content. > > Signed-off-by: Pete Batard Reviewed-by: Thomas Schmitt Tested-by: Thomas Schmitt I reviewed (modulo style aspects) and applied the patch [2/3] to a freshly pulled grub-mkrescue.c . It compiles and produces (on Debian) an ISO with: grub/grub-mkrescue -d /usr/lib/grub/x86_64-efi \ --locale-directory=/usr/share -o test.iso minimal (It would be more realistic if i knew how the Debian binary of grub-mkrescue knows that there is also /usr/lib/grub/i386-pc and combines it with x86_64-efi. If i use two -d options, then the latter overrides the former.) Whatever, the resulting test.iso has EFI boot equipment of the same size as the ISO from Debian's grub-mkrescue with the same arguments as above. Different than with Debian's binary, there is now a directory tree /efi in the ISO with 146 KiB of storage space (measured by xorriso -dus). Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/3] Add support for EFI file system transposition
Hi, Pete Batard wrote: > 2. It uses a efi.img to embed the UEFI bootloaders, but does not keep a copy > of these bootloaders on the ISO9660 file system itself, with the end result > that, when copying the media at the file system level, the '/efi/boot/' > directory and its content is missing. I understand why this is desirable for the use case of preparing a grub-mkrescue USB stick on MS-Windows. Insofar i support the duplication of the FAT image content in the ISO 9660 filesystem. > we will point out that we consider it > should really be the job of xorriso, rather than grub-mkrescue, to > accomplish this duplication (hence why I am CC'ing Thomas), but we don't > know the technical difficulties that result from trying to map back the > content of a FAT image back onto the ISO9660 structure. xorriso would have to learn to unpack FAT filesystems. But FAT is not really the topic of xorriso. Given that the FAT filesystem is freshly composed by grub-mkrescue from a readily prepared file tree on disk, i deem it more straightforward that grub-mkrescue simply tells xorriso to put this tree into the ISO. Either implicitely by having it in iso9660_dir (as patch [2/3] proposes) or explicitely by a pathspec (like /efi=...temporary.disk.path...). In the latter case the temporary disk file tree has to survive until the xorriso run is finished. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] tests: Refactor building xorriso command for iso9660 tests
Hi, On Fri, 10 Dec 2021 02:41:44 -0600, Glenn Washburn wrote: > The iso9660 tests test creating isos with different combinations of joliet, > rockridge, and iso9660 conformance level. Refactor xorriso argument > generation for more readability and extensibility. > > Signed-off-by: Glenn Washburn > --- > Updates since v1: > * Reorder such that command line arguments are built in order, per Thomas' > suggestion > --- Reviewed-by: Thomas Schmitt Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] tests: Refactor building xorriso command for iso9660 tests
Hi,
i think this change is beneficial for the maintainability of the test.
But this sequence looks a bit confusing, albeit it is ok on the second
glimpse:
+ XORRISO_ARGS="-as mkisofs $XORRISOFS_CHARSET -graft-points"
+
+ if [ -z "${fs##*rockridge*}" ]; then
+ XORRISO_ARGS="-rockridge on $XORRISO_ARGS"
+ else
+ XORRISO_ARGS="-rockridge off $XORRISO_ARGS"
+ fi
+
+ if [ -z "${fs##*1999*}" ]; then
+ XORRISO_ARGS="$XORRISO_ARGS -iso-level 4"
+ else
+ XORRISO_ARGS="$XORRISO_ARGS -iso-level 3"
+ fi
It is essential here, but not really obvious, that the native command
-rockridge "on"|"off" must be _prepended_ to XORRISO_ARGS whereas the
mkisofs emulation options must be _appended_ to the variable content.
If not, then the xorriso run would fail:
$ xorriso -as mkisofs -rockridge on
...
xorriso : FAILURE : -as mkisofs: Unrecognized option '-rockridge'
xorriso : aborting : -abort_on 'FAILURE' encountered 'FAILURE'
$ xorriso -iso-level 4 -as mkisofs
...
xorriso : FAILURE : Not a known command: '-iso-level'
xorriso : FAILURE : Not a known command: '4'
xorriso : aborting : -abort_on 'FAILURE' encountered 'FAILURE'
$
It would be more intuitive to build XORRISO_ARGS in the sequence that
will be seen by xorriso.
So consider to pull -compliance "rec_mtime" into XORRISO_ARGS too, and
to set the native commands before the -as "mkisofs" command.
Like:
XORRISO_ARGS="-compliance rec_mtime"
if [ -z "${fs##*rockridge*}" ]; then
XORRISO_ARGS="$XORRISO_ARGS -rockridge on"
else
XORRISO_ARGS="$XORRISO_ARGS -rockridge off"
fi
XORRISO_ARGS="$XORRISO_ARGS -as mkisofs $XORRISOFS_CHARSET
-graft-points"
if [ -z "${fs##*1999*}" ]; then
XORRISO_ARGS="$XORRISO_ARGS -iso-level 4"
else
XORRISO_ARGS="$XORRISO_ARGS -iso-level 3"
fi
if [ -z "${fs##*joliet*}" ]; then
XORRISO_ARGS="$XORRISO_ARGS -J -joliet-long"
fi
xorriso $XORRISO_ARGS -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] Make a "gdb" dprintf that tells us load addresses.
Hi, Robbie Harwood wrote: > dl.c:694: > add-symbol-file ... > dl.c:694: > add-symbol-file ... > dl.c:694: > add-symbol-file ... > > You can't easily copy-paste that into gdb. It needs to be processed to > remove the file:line stuff. How about hiding the "dl.c" lines behind a "#" ? # dl.c:694: add-symbol-file ... I don't find the current gdb documentation online from GNU, but https://sourceware.org/gdb/current/onlinedocs/gdb/Command-Syntax.html#Command-Syntax looks young and promises "Any text from a # to the end of the line is a comment; it does nothing." (All gdbs i ever saw fulfill this promise.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2] tests: Keep grub-fs-tester ziso9660 from failing for wrong reasons
The test for the ability to decompress zisofs encoded files is supposed
to fail due to the lack of this ability in GRUB. But it fails early with
xorriso : FAILURE : -volid: Text too long (1650 > 32)
because "ziso9660" is not in the list of filesystems which accept at most
32 bytes in their FSLABEL.
If this is fixed, the test returns false success because the xorriso run
does not produce any zisofs compressed files. The problem is in the
sequence of native xorriso commands used. The command -set_filter_r
applies only to the files which are already inserted into the emerging
ISO filesystem. In the current sequence no files have been inserted yet
by command -add when the last of two -set_filter_r commands is executed.
After this is corrected, xorriso refuses to work because the global
settings of command -zisofs can be made only before command -set_filter_r
has attached zisofs filters to the data files in the emerging ISO.
Further: A bug in xorriso causes a false warning about FSLABEL being
too long for Joliet. Shortcommings of Joliet cause warnings about
symbolic links. Such warnings might distract from the actual reason why
the test is expected to fail.
So add "ziso9660" to the 32-byte FSLABEL list.
Fix the xorriso run to produce compressed files which for now cause
righteous failure of the test. Do this by removing a surplus group of
-set_filter_r and -zisofs commands, by moving the other such group
behind -add, and by swapping -set_filter_r and -zisofs.
Remove the -as mkisofs options which produce a Joliet filesystem tree.
Signed-off-by: Thomas Schmitt
---
tests/util/grub-fs-tester.in | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index 4f581b638..a28e07295 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -318,7 +318,8 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE"
1); do
FSLABEL="grub_;/testéтi
u😁莽茝кириrewfceniuewruevrewnuuireurevueurnievrewfne";;
# FS LIMITATION: afs and iso9660 label is at most 32 UTF-8
characters
x"afs" | xiso9660 | xrockridge | xrockridge_joliet\
- | xiso9660_1999 | xrockridge_1999 |
xrockridge_joliet_1999)
+| xiso9660_1999 | xrockridge_1999\
+| xrockridge_joliet_1999 | xziso9660)
FSLABEL="gr_;/é莭莽😁кирит u";;
# FS LIMITATION: bfs label is at most 32 UTF-8 characters
# OS LIMITATION: bfs label can't contain ; or /
@@ -1024,7 +1025,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
(cd "$MASTER"; find . | cpio -o -H "$(echo ${fs} | sed
's@^cpio_@@')" > "${FSIMAGEP}0.img" ) ;;
x"ziso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso -compliance rec_mtime -set_filter_r --zisofs --
-zisofs default -as mkisofs $XORRISOFS_CHARSET -iso-level 3 -graft-points -R -J
-joliet-long -V "$FSLABEL" --modification-date=$(echo ${FSUUID} | sed
's/-//g;') -o "${FSIMAGEP}0.img" -- -set_filter_r --zisofs -- -zisofs default
-add /="$MASTER" ;;
+ xorriso -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -R -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img" --
-add /="$MASTER" -- -zisofs default -set_filter_r --zisofs / -- ;;
x"iso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
--
2.20.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] [PATCH] tests: Keep grub-fs-tester ziso9660 from failing for wrong reasons
Hi, i am working on the requested changes. In my patch message i wrote: > > A bug > > in xorriso causes a distracting warning about FSLABEL being too long for > > Joliet. Shortcommings of Joliet cause warnings about symbolic links. Daniel Kiper wrote: > s/links./links too./? Rather not. Both warnings have very different reasons. The xorriso bug was that the warning came if more than 16 bytes of volume id are submitted. The correct test is to count the UCS-2 characters after conversion from the local character set (meanwhile surely UTF-8). The FSLABEL which is prepared by grub-fs-tester for ISO 9660 tests has only 15 characters but 32 UTF-8 bytes. The shortcomming of Joliet is in its specs. It was invented by Microsoft Inc. for presenting Microsoft filesystem names. At least back in 1995 no symbolic links were desired for their version of ISO 9660. Other than the xorriso bug i cannot fix this. Both warnings could deceive the reader of the test report, especially since the test is expected to fail. Thus my patch disables Joliet to silence both for all xorriso versions. > I have an itching to ask you to split this thing into two patches. I decided against this because the test would indicate false success after the early failure because of the FSLABEL length is fixed. If i fix the xorriso command sequence problem first, then this first patch does not have any effect at run time. So i rather consider the ziso9660 test as untested sketch which i replace by a working test with proper failure as long as no zisofs decompression is implemented. > You described everything in the commit message what you are doing here > except of "-set_filter_r --zisofs -- -zisofs default" games. Could you > explain that too? Will try without copying a whole paragraph from man xorriso. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] [PATCH] tests: Keep grub-fs-tester ziso9660 from failing for wrong reasons
Hi, sorry for messing up the mail subject by "[PATCH] [PATCH]". (I added one of them already to the local commit message.) Shall i send new mail with better subject ? Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] [PATCH] tests: Keep grub-fs-tester ziso9660 from failing for wrong reasons
The test for the ability to decompress zisofs encoded files is supposed
to fail due to the lack of this ability in GRUB. But it fails early with
xorriso : FAILURE : -volid: Text too long (1650 > 32)
because "ziso9660" is not in the list of filesystems which accept at most
32 bytes in their FSLABEL. If this is fixed, the test returns false success
because the xorriso run does not produce any zisofs compressed files. A bug
in xorriso causes a distracting warning about FSLABEL being too long for
Joliet. Shortcommings of Joliet cause warnings about symbolic links.
So add "ziso9660" to the 32-byte FSLABEL list. Fix the xorriso run to
produce compressed files which for now cause righteous failure of the test.
Remove the option to produce a Joliet filesystem tree in order to
concentrate on testing zisofs decompression.
Signed-off-by: Thomas Schmitt
---
tests/util/grub-fs-tester.in | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index 4f581b638..a28e07295 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -318,7 +318,8 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE"
1); do
FSLABEL="grub_;/testéтi
u😁莽茝кириrewfceniuewruevrewnuuireurevueurnievrewfne";;
# FS LIMITATION: afs and iso9660 label is at most 32 UTF-8
characters
x"afs" | xiso9660 | xrockridge | xrockridge_joliet\
- | xiso9660_1999 | xrockridge_1999 |
xrockridge_joliet_1999)
+| xiso9660_1999 | xrockridge_1999\
+| xrockridge_joliet_1999 | xziso9660)
FSLABEL="gr_;/é莭莽😁кирит u";;
# FS LIMITATION: bfs label is at most 32 UTF-8 characters
# OS LIMITATION: bfs label can't contain ; or /
@@ -1024,7 +1025,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
(cd "$MASTER"; find . | cpio -o -H "$(echo ${fs} | sed
's@^cpio_@@')" > "${FSIMAGEP}0.img" ) ;;
x"ziso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso -compliance rec_mtime -set_filter_r --zisofs --
-zisofs default -as mkisofs $XORRISOFS_CHARSET -iso-level 3 -graft-points -R -J
-joliet-long -V "$FSLABEL" --modification-date=$(echo ${FSUUID} | sed
's/-//g;') -o "${FSIMAGEP}0.img" -- -set_filter_r --zisofs -- -zisofs default
-add /="$MASTER" ;;
+ xorriso -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -R -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img" --
-add /="$MASTER" -- -zisofs default -set_filter_r --zisofs / -- ;;
x"iso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
--
2.20.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 1/8] tests: Make sure LANG is set properly for iso9660_test
Hi, i wrote: > > it might be contraproductive to set LANG=en_US.UTF-8. Glenn Washburn wrote: > My patch only sets LANG if empty, so in this case I don't think its > counterproductive, just not productive. Yes. It cautiously avoids to make the situation any worse. I meant my own announced plan which was more agressively aiming for enforcing UTF-8. Well, i hope that my cheat sheet for (unsuccessful) kernel contribution attempts enabled me to create a convincing imitation of an acceptable patch for GRUB. To help reading the oversized line salad: I defined the variable XORRISOFS_CHARSET and added $XORRISOFS_CHARSET after each "-as mkisofs" in the xorriso command lines. Tests were made without any LANG setting (effectively en_US.utf-8), with LANG="", "C", "en_US.ISO-8859-1" (not installed), "en_US.UTF-8", "de_DE.UTF-8" (not installed). All succeeded. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] tests: Let xorriso fixely assume UTF-8 as local character set
iso9660_test fails if the effective locale is not UTF-8. This happens
because xorriso needs to convert file names and FSLABEL to UCS-2 when
preparing a Joliet tree. grub-fs-tester obviously intends to use UTF-8
as character set, but xorriso assumes by default the result of
nl_langinfo(3) with item CODESET.
So override the result of nl_langinfo(CODESET) by options of xorriso -as
mkisofs.
Signed-off-by: Thomas Schmitt
---
tests/util/grub-fs-tester.in | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index bfc425e1f..4f581b638 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -8,6 +8,10 @@ GRUBFSTEST="@builddir@/grub-fstest"
tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+# xorriso -as mkisofs options to ignore locale when processing file names and
+# FSLABEL. This is especially needed for the conversion to Joliet UCS-2.
+XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
+
# This wrapper is to ease insertion of valgrind or time statistics
run_it () {
LC_ALL=C "$GRUBFSTEST" "$@"
@@ -1020,31 +1024,31 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
(cd "$MASTER"; find . | cpio -o -H "$(echo ${fs} | sed
's@^cpio_@@')" > "${FSIMAGEP}0.img" ) ;;
x"ziso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso -compliance rec_mtime -set_filter_r --zisofs --
-zisofs default -as mkisofs -iso-level 3 -graft-points -R -J -joliet-long -V
"$FSLABEL" --modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o
"${FSIMAGEP}0.img" -- -set_filter_r --zisofs -- -zisofs default -add
/="$MASTER" ;;
+ xorriso -compliance rec_mtime -set_filter_r --zisofs --
-zisofs default -as mkisofs $XORRISOFS_CHARSET -iso-level 3 -graft-points -R -J
-joliet-long -V "$FSLABEL" --modification-date=$(echo ${FSUUID} | sed
's/-//g;') -o "${FSIMAGEP}0.img" -- -set_filter_r --zisofs -- -zisofs default
-add /="$MASTER" ;;
x"iso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
-iso-level 3 -graft-points -V "$FSLABEL" --modification-date=$(echo ${FSUUID} |
sed 's/-//g;') -o "${FSIMAGEP}0.img" /="$MASTER" ;;
+ xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
x"joliet")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
-iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
+ xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
x"rockridge")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
-iso-level 3 -graft-points -V "$FSLABEL" --modification-date=$(echo ${FSUUID} |
sed 's/-//g;') -o "${FSIMAGEP}0.img" /="$MASTER" ;;
+ xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
x"rockridge_joliet")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
-iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
+ xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
x"iso9660_1999")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
-
Re: [PATCH v2 1/8] tests: Make sure LANG is set properly for iso9660_test
Hi, it turns out that it might be contraproductive to set LANG=en_US.UTF-8. If the LANG locale is not listed by locale -a then nl_langinfo(3) returns "ANSI_X3.4-1968". I stumbled over this when testing LANG=de_DE.UTF8, which despite of my location is not an available locale on my machine. So i think that setting any locale by LANG makes daring assumptions about its availability, unless brain is added to inspect the installed locals and to choose one that is based on UTF-8. This reaches quite far beyond the problem which the patch shall solve. What route to go ? - Choose the first UTF-8 locale offered by locale -a ? (What to do if none is offered ?) - Abandon the idea of setting LANG and rather use xorrisofs charset options to enforce UTF-8 as origin of the conversion ? I will for now strive for the second alternative but am ready to change to number one. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [grub-fs-tester.in] zisofs test looks unsuitable
Hi. Glenn Washburn wrote: > I think the changes to get the test > working are worthy of inclusion so that the tests are ready when this > feature gets implemented. Do you think my draft of a commit message is ok ? Probably i should mention that this test will fail until zisofs is implemented. > I'm not familiar with that code nor the > format details, but it sounds like you might be able to determine if > its something somewhat easy to add to GRUB. Well, i wrote an implementation as libisofs filter (which differs much from a filesystem driver) and a documentation of the format: https://sources.debian.org/src/libisofs/1.5.2-1/doc/zisofs_format.txt (Since noon our project web server is down. I need to pester the web master to check which useless piece of software crashed it this time.) Implementing SUSP entry "ZF" would be a substantial change to grub-core/fs/iso9660.c including a new dependency on zlib. The adventure would probably begin at function susp_iterate_dir() which interprets some Rock Ridge SUSP entries. Then i'd need to know which functions tell the file size and do the reading of file content data and how to make the info from the ZF entry known to them. The Linux kernel has an implementation from the inventor, H.Peter Anvin. (With a bug for machines with PAGE_SIZE > 32 KB https://lore.kernel.org/linux-scsi/20201120140633.1673-1-scdbac...@gmx.net/T/#u and a dire need for a readahead implementation.) And there is the successor format zisofs2, not yet in the kernel https://fossies.org/linux/misc/xorriso-1.5.4.pl02.tar.gz/xorriso-1.5.4/doc/zisofs2_format.txt?m=t (Thanks to all the sites which store what i write.) Last november a zisofs2 patch of ~200 lines enabled the then actual 5.10.0 release candidate to read the new format. But nearly a year of bitrot already devalued my readahead patch. So my local test kernel is the only one in the world which can read giant zisofs2 compressed files at high speed. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 1/8] tests: Make sure LANG is set properly for iso9660_test
Hi,
i wrote:
> > xorriso ... -as mkisofs -input-charset UTF-8 -output-charset UTF-8
Glenn Washburn wrote:
> Do there need to be any UTF-8 locales installed (or any locales for
> that matter) for this to work? My guess is no.
I expect the same. It's not easy to test, though.
In theory it is essentially a matter of iconv_open(3) and iconv(3) whether
conversion works.
The installed locale only gives the default values for parameters of
iconv_t iconv_open(const char *tocode, const char *fromcode);
The options -input-charset and -output-charset override the locale
setting, whatever it is.
(Actually xorriso stumbled over the hardcoded output character set of
Joliet, which is UCS-2.)
> Correct me if I'm wrong, but this logic about defaulting LANG is more
> general than the iso9660_test. Would you agree that this should be
> applied to the tests a a whole?
The comments in grub-fs-tester talk of UTF-8 as expected character set
and the text constants look like UTF-8, too. So it would not be wrong
to enforce a UTF-8 locale.
It might be peculiar to xorriso to look at the locale when interpreting
the meaning of bytes which represent characters.
But this interpretation is inevitable if the target filesystem specs
prescribe a particular character set, as Joliet does, and if we aim for
dealing with UTF-8 names.
> If yes, then I should probably remove this patch altogether and put
> this code elsewhere.
I think it can be justified to hardcode UTF-8 in grub-fs-tester regardless
whether higher levels of the test empire or the user have own interests
in locale settings.
The code in grub-fs-tester is a good example for the situation that the
file names on disk might belong to users with differing locales. We expect
UTF-8 to be the common base of all those nowadays.
So i'd vote for something like
if [ -n "${LANG##*UTF*}" ]; then
echo "NOTE: LANG=$LANG appears to not be UTF-8."
echo "NOTE: Setting LANG=en_US.UTF-8 to match the test file names."
export LANG=en_US.UTF-8
fi
at the start of grub-fs-tester.in.
> > How can i force "make check" to do something at its second run ?
> However, "make check" should still be running the test.
I obviously got misled by the many make messages and the silence of the
iso9660_test run.
> Would you submit a patch with the changes you've
> outlined that would fix this issue for when LANG is empty?
I'd go farer and smack down everything that does not claim to be UTF-8.
Is a git diff enough or has it to be git format-patch from a local commit ?
Is git send-email necessary ?
(My relation to git based interaction with projects is best illustrated by
https://upload.wikimedia.org/wikipedia/commons/thumb/1/17/Laocoon_Pio-Clementino_Inv1059-1064-1067.jpg/564px-Laocoon_Pio-Clementino_Inv1059-1064-1067.jpg
)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [grub-fs-tester.in] zisofs test looks unsuitable
Hi,
i managed to get the ziso9660 test running with actual compression.
./grub-fs-tester ziso9660
now produces an ISO image but seems not to be happy with it.
If i get it right then it sees the compressed size 1124519 of file "1.img"
but expects its uncompressed size 5242879.
Linux mount sees
-rw-r--r-- 2 thomas thomas 5242879 Aug 26 21:52 1.img
So grub-fstest.c CMD_LS seems not yet ready for dealing with zisofs.
This is the change which brought me that far, including a draft for a
commit message:
The grub-fs-tester test "ziso9660" failed early with a xorriso error
because it was not in the list of tests which tolerate at most 32 bytes
of FSLABEL.
The xorriso run of ziso9660 did not actually compress the data files because
the xorriso command for installing zisofs filters was applied before any
files had populated the emerging ISO image.
So add "ziso9660" where other ISO 9660 tests get a shorter FSLABEL and
re-arrange the commands of the xorriso run to bring -add and -zisofs before
-set_filter_r.
Signed-off-by: Thomas Schmitt
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index bfc425e1f..834c05728 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -314,7 +314,8 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE"
1); do
FSLABEL="grub_;/testéтi
u😁莽茝кириrewfceniuewruevrewnuuireurevueurnievrewfne";;
# FS LIMITATION: afs and iso9660 label is at most 32 UTF-8
characters
x"afs" | xiso9660 | xrockridge | xrockridge_joliet\
- | xiso9660_1999 | xrockridge_1999 |
xrockridge_joliet_1999)
+ | xiso9660_1999 | xrockridge_1999\
+ | xrockridge_joliet_1999 | xziso9660)
FSLABEL="gr_;/é莭莽😁кирит u";;
# FS LIMITATION: bfs label is at most 32 UTF-8 characters
# OS LIMITATION: bfs label can't contain ; or /
@@ -1020,7 +1021,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
(cd "$MASTER"; find . | cpio -o -H "$(echo ${fs} | sed
's@^cpio_@@')" > "${FSIMAGEP}0.img" ) ;;
x"ziso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso -compliance rec_mtime -set_filter_r --zisofs --
-zisofs default -as mkisofs -iso-level 3 -graft-points -R -J -joliet-long -V
"$FSLABEL" --modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o
"${FSIMAGEP}0.img" -- -set_filter_r --zisofs -- -zisofs default -add
/="$MASTER" ;;
+ xorriso -compliance rec_mtime -as mkisofs -iso-level 3
-graft-points -R -J -joliet-long -V "$FSLABEL" --modification-date=$(echo
${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img" -- -add /="$MASTER" -- -zisofs
default -set_filter_r --zisofs / -- ;;
x"iso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
xorriso --rockridge off -compliance rec_mtime -as mkisofs
-iso-level 3 -graft-points -V "$FSLABEL" --modification-date=$(echo ${FSUUID} |
sed 's/-//g;') -o "${FSIMAGEP}0.img" /="$MASTER" ;;
Here is a run with this change:
$ ./grub-fs-tester ziso9660
GNU xorriso 1.5.4.pl02 : RockRidge filesystem manipulator, libburnia project.
Drive current: -outdev 'stdio:/tmp/tmp.iIXC5zIfEd/ziso9660_512_512_1_0.img'
Media current: stdio file, overwriteable
Media status : is blank
Media summary: 0 sessions, 0 data blocks, 0 data, 400g free
xorriso : WARNING : -volid text problematic as automatic mount point name
xorriso : WARNING : -volid text is too long for Joliet (31 > 16)
xorriso : WARNING : -volid text does not comply to ISO 9660 / ECMA 119 rules
Added to ISO image: directory '/'='/tmp/tmp.iIXC5zIfEd/master'
xorriso : UPDATE : 227 files added in 1 seconds
xorriso : UPDATE : 10 file filters processed in 1 seconds
libisofs: WARNING : Cannot add /longsym to Joliet tree. Symlinks can only be
added to a Rock Ridge tree.
libisofs: WARNING : Cannot add /psym to Joliet tree. Symlinks can only be added
to a Rock Ridge tree.
libisofs: WARNING : Cannot add /sdir/ssym to Joliet tree. Symlinks can only be
added to a Rock Ridge tree.
libisofs: WARNING : Cannot add /sdir/usym to Joliet tree. Symlinks can only be
added to a Rock Ridge tree.
libisofs: WARNING : Cannot add /sym to Joliet tree. Symlinks can only be added
to a Rock Ridge tree.
libisofs: WARNI
