How to test grub-install from git tree without "make install" ?

Hi, i made a hopefully harmless change to grub-install-common.c in order to avoid that backup files of /boot/grub/locale/*.mo get copied into the resulting ISO image of grub-mkrescue when more than one platform-target combination is configured. E.g. "x86_64-efi" together with "i386-pc". Begin of t

Re: [PATCH v2 0/6] Various test fixes proposed by Thomas Schmitt

Hi, Tested-by: Thomas Schmitt with remaining unrelated problems. --- Test details: The patch series applied by "git am" without problems to a branch cloned from freshly pulled "master".

Re: [SECURITY PATCH 18/73] fs/ntfs: Implement attribute verification

Hi, i wrote: > > it is quite some hurdle that some of the tests need to be run as > > superuser. Glenn Washburn wrote: > Yes, not ideal. I have spent an embarrassing amount of time developing > scripts that do away with with root requirement, and it works. The way > they are able to run the root

Re: [SECURITY PATCH 18/73] fs/ntfs: Implement attribute verification

Hi, Glenn Washburn wrote: > Seems like running them the tests before > committing large patch series, like this security update, might be a > good idea. I think that any change of the filesystem code should be tested as much as possible. But it is quite some hurdle that some of the tests need to

Re: [SECURITY PATCH 18/73] fs/ntfs: Implement attribute verification

Hi, Glenn Washburn wrote: > Seems like running them the tests before > committing large patch series, like this security update, might be a > good idea. I think that any change of the filesystem code should be tested as much as possible. But it is quite some hurdle that some of the tests need to

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

Hi, Glenn Washburn wrote: > > > if [ -z "$debug" ] && [ "$RET" -eq "$xfail" ]; then I wrote: > > If indeed in a successful --xfail test RET must have the value of "$xfail" > > (i guess 1), then it is concise and sufficient. > [...] the only acceptable xfail case is for RET=1. If > RET is greate

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

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 cl

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-l

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

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 >

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 > un

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 w

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 t

Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /

ar 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

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 th

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 t

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

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

[PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /

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

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 u

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:'" \ @bu

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

[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

[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

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 iso966

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

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 und

[PATCH 0/2] grub-fstest: Show error message if command causes grub_errno

: 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

[PATCH 2/2] tests: Use new grub-fstest option -E with iso9660_ce_loop test

-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

[PATCH 1/2] grub-fstest: Show error message if command causes grub_errno

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

[PATCH 1/1] util/grub-mkrescue: Check existence of option arguments

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

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 break

[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 i

[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 'phco

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 whi

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 i

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" beca

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 f

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

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.

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

[PATCH 2/2] tests: Add test for iso9660 delayed CE hop

test. 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_e

[PATCH 1/2] fs/iso9660: Delay CE hop until end of current SUSP area

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

[PATCH 0/2] fs/iso9660: Delay CE hop until end of current SUSP area

erse 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: A

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

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

Re: [PATCH 2/2] docs: Document that the functional test requires the package xfonts-unifont

r 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

t; > +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-qem

Re: [PATCH v3] tests: Add pathological iso9660 filesystem tests

;@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 > + te

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.

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: ...

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 th

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 usa

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. ...

Re: [PATCH v2] tests: Add pathological iso9660 filesystem tests

uilddir@/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"; ex

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_199

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

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. A

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 ? Whatev

Re: [PATCH v3 5/5] fs/iso9660: Prevent skipping CE or ST at start of continuation area

P 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(+) > >

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

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 und

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

Re: [PATCH v2 5/5] fs/iso9660: Prevent skipping CE or ST at start of continuation area

P 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/g

Re: [PATCH v2 4/5] fs/iso9660: Incorrect check for entry boundary

zeof (*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

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 inser

Re: [PATCH v2 2/5] fs/iso9660: Prevent read past the end of system use area

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_e

Re: [PATCH v2 1/5] fs/iso9660: Add check to prevent infinite loop

f 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

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

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: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb

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

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

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 wo

Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary

ec 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->

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. Th

Proposal v2: fs/iso9660: Prevent skipping CE or ST at start of continuation area

tries 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-1

Proposal: fs/iso9660: Prevent skipping CE or ST at start of continuation area

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

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 abo

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 in

Re: [PATCH 3/4] fs/iso9660: Avoid reading past the entry boundary

> 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 #defi

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.

Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop

XTENTS; > + 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 necess

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

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: <5036388200

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

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 a

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

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

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 H

Re: [PATCH 2/3] grub-mkrescue: Preserve a copy of the EFI bootloaders on the ISO9660 file system

9660 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 revie

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 u

Re: [PATCH v2] tests: Refactor building xorriso command for iso9660 tests

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-

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 +

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 behin

[PATCH v2] tests: Keep grub-fs-tester ziso9660 from failing for wrong reasons

-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..a28e0

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 wa

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://

[PATCH] [PATCH] tests: Keep grub-fs-tester ziso9660 from failing for wrong reasons

iso9660" 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/uti

  1   2   3   >