[PATCH] Filter out POSIX locale for translation
The POSIX locale is default or native operating system's locale
identical to the C locale, so no translation to human speaking languages
provided.
For this reason we should filter out LANG=POSIX as well as LANG=C upon
generating grub.cfg to avoid looking up for it's gettext's message
catalogs that will consequently result in the unpleasant message.
error: file `/boot/grub/locale/POSIX.gmo' not found
Signed-off-by: Michael Chang
---
util/grub.d/00_header.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
index 93a90233e..f74c2a4c6 100644
--- a/util/grub.d/00_header.in
+++ b/util/grub.d/00_header.in
@@ -191,7 +191,7 @@ EOF
EOF
# Gettext variables and module
-if [ "x${LANG}" != "xC" ] && [ "x${LANG}" != "x" ]; then
+if [ "x${LANG}" != "xC" ] && [ "x${LANG}" != "xPOSIX" ] && [ "x${LANG}" != "x"
]; then
cat << EOF
set locale_dir=\$prefix/locale
set lang=${grub_lang}
--
2.31.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Print module name on license check failure
On Mon, Oct 25, 2021 at 06:17:03PM -0400, Robbie Harwood wrote:
> Before performing the license check, resolve the module name so that it
> can be printed if the license check fails. Prior to this change, grub
> would only indicate that the check had been failed, but not by what
> module. This made it difficult to track down either the problem module,
> or debug the false positive further.
>
> Signed-off-by: Robbie Harwood
> ---
> grub-core/kern/dl.c | 21 ++---
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 48f8a7907..363bacc43 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -457,14 +457,21 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
> Be sure to understand your license obligations.
> */
> static grub_err_t
> -grub_dl_check_license (Elf_Ehdr *e)
> +grub_dl_check_license (grub_dl_t mod, Elf_Ehdr *e)
> {
>Elf_Shdr *s = grub_dl_find_section (e, ".module_license");
> - if (s && (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") == 0
> - || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3+") == 0
> - || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv2+") == 0))
> + if (!s)
s == NULL please...
> +return grub_error (GRUB_ERR_BAD_MODULE,
> +"no license section in module %.64s", mod->name);
> +
> + if (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") == 0
> + || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3+") == 0
> + || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv2+") == 0)
Could use grub_strncmp() instead of grub_strcmp() here?
> return GRUB_ERR_NONE;
> - return grub_error (GRUB_ERR_BAD_MODULE, "incompatible license");
> +
> + return grub_error (GRUB_ERR_BAD_MODULE,
> + "incompatible license in module %.64s: %.64s", mod->name,
> + (char *) e + s->sh_offset);
> }
>
> static grub_err_t
> @@ -641,8 +648,8 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
> constitutes linking) and GRUB core being licensed under GPLv3+.
> Be sure to understand your license obligations.
>*/
> - if (grub_dl_check_license (e)
> - || grub_dl_resolve_name (mod, e)
> + if (grub_dl_resolve_name (mod, e)
> + || grub_dl_check_license (mod, e)
I think you should split this patch into two. One which improves error
messages and another which imposes length restrictions on the
grub_strcmp() and grub_error() in the grub_dl_check_license(). And
I think you should use 63 instead of 64 (63 + NUL gives us 64 :-)).
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Filter out POSIX locale for translation
On Tue, Oct 26, 2021 at 03:11:00PM +0800, Michael Chang via Grub-devel wrote: > The POSIX locale is default or native operating system's locale > identical to the C locale, so no translation to human speaking languages > provided. > > For this reason we should filter out LANG=POSIX as well as LANG=C upon > generating grub.cfg to avoid looking up for it's gettext's message > catalogs that will consequently result in the unpleasant message. > > error: file `/boot/grub/locale/POSIX.gmo' not found > > Signed-off-by: Michael Chang Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Multiboot ELF Loader - Alignment
Hey, On Thu, Oct 21, 2021 at 08:46:06PM +0100, Chris Plant wrote: > Hello, > > I'm continuing to play with Multiboot2 on ARM64 on a Raspberry Pi, and > I've run into something which I'm trying to understand. Why do you use Multiboot2 on ARM64? > I have an ELF file for my kernel which has two segments (I have no idea > why rust is giving me two segments, but it is). If I boot directly > into the ELF file from the Pi firmware it boots fine, but if I boot via > GRUB I have issues with data corruption in .rodata which is in the > second segment. The first segment (.text) appears to load correctly. Could you share the output from "readelf -Wa "? Additionally, how is your Multiboot2 header defined in the kernel? > Digging into this, the ELF headers specify a load base address of > 0x801060 for the second segment, but GRUB allocates and loads this to > 0x802000. If I change my linker to align the second segment onto > 0x1000 everything works fine. The Multiboot2 protocol does not tolerate "holes" in the image. You can find good explanation what I mean by that here [1]. > Is this alignment to 0x1000 a defined behaviour of the GRUB allocator > or Multiboot2? I'm assuming it's not a problem with the ELF file as > it's generated by usual means and runs fine otherwise. I think you should take closer look at grub-core/loader/multiboot_elfxx.c and especially lines starting from 258. It seems to me something around here overwrites part of the image in the memory. Daniel [1] https://lists.xenproject.org/archives/html/xen-devel/2021-05/msg01003.html ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] i386-pc: build btrfs zstd support into separate module
On Fri, Sep 10, 2021 at 05:22:22PM +0800, Michael Chang via Grub-devel wrote: > On Wed, Sep 08, 2021 at 09:37:52PM +0200, Daniel Kiper wrote: > > On Fri, Sep 03, 2021 at 09:21:39AM +0800, Michael Chang via Grub-devel > > wrote: > > > On Thu, Sep 02, 2021 at 02:12:52PM +0200, Daniel Kiper wrote: > > > > On Thu, Sep 02, 2021 at 01:48:30PM +0800, Michael Chang via Grub-devel > > > > wrote: > > > > > On Wed, Sep 01, 2021 at 06:38:22PM +0200, Daniel Kiper wrote: > > > > > > On Tue, Aug 31, 2021 at 03:12:28PM +0800, Michael Chang via > > > > > > Grub-devel wrote: > > [snip] > > > > just that I can't resist to fix problem from our users who opted to use > > > "btrfs partition" which differs to "short mbr gap" with a lot more user > > > base and sensible usecases. > > > > > > FWIW we want to address this problem, because mbr gap is adjustable via > > > re-partitioning but btrfs bootloader area is not. > > > > Huh! The "btrfs partition" sounds like not resizable MBR gap! I am not > > happy with it just at the beginning. Anyway, could you explain your use > > case in more details including example commands and why core.img seems > > landing in the "btrfs partition". I am especially curious about the > > latter because I think the "btrfs partition" and things like that was > > designed for something else, e.g., storing grubenv data. And why this > > solution is i386 specific? > > Installing to btrfs partition is not something exotic to grub, it is > actually a supported usecase by grub for a long time. Also zfs has > similar design, see: > > c7ba4f698 Support BtrFS embedding. > ba102053c Support ZFS embedding. > > To make it happen, you just have to point the btrfs device to > grub-install and it will setup and embed image there. That worked quite > nicely until the zstd support came into play. > > On current git head, the command fails with core.img is too large. > > # ./grub-install -d ./grub-core /dev/vda2 > Installing for i386-pc platform. > ./grub-install: warning: your core.img is unusually large. It won't fit in > the embedding area. > ./grub-install: error: filesystem `btrfs' doesn't support blocklists. > > With this patch, the embedding still works but warns the user zstd is > disabled, also instructing them to use MBR if they need the zstd feature > to work. > > # ./grub-install -d ./grub-core /dev/vda2 > Installing for i386-pc platform. > ./grub-install: warning: btrfs zstd compression is disabled, please change > install device to disk. > Installation finished. No error reported. > > It is very i386-pc centric, as it is often used by legacy mbr boot code > with which active partition is chainloaded. Some people still thinks > this is the right thing to do and feels more comfortable this way to > manage their multiple distrubution booting, compared with os-prober. I do not like this solution because it is similar to all the patches trying to solve small MBR problem. Though, after chatting with Btrfs folks in Oracle, it seems to me there is better solution for this issue. I think we should make the GRUB installer smarter. If it cannot put the core image in first 64 KiB before primary superblock it should install the core image behind the primary superblock, i.e. starting from 64 KiB + 4 KiB but below 1 MiB. Please take a look at [1] which nicely explains the Btrfs bootloader support. Does it make sense for you? Daniel [1] https://btrfs.wiki.kernel.org/index.php/Manpage/btrfs(5)#BOOTLOADER_SUPPORT ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Multiboot ELF Loader - Alignment
Thanks for the detailed reply, I think I’ve got to the bottom of this now, I realised what I missed just after I sent this email. > On 26 Oct 2021, at 13:35, Daniel Kiper wrote: > > Hey, > >> On Thu, Oct 21, 2021 at 08:46:06PM +0100, Chris Plant wrote: >> Hello, >> >> I'm continuing to play with Multiboot2 on ARM64 on a Raspberry Pi, and >> I've run into something which I'm trying to understand. > > Why do you use Multiboot2 on ARM64? I guess the question is why wouldn’t I? Is the assumption that I should use ARM32 and switch after booting? > >> I have an ELF file for my kernel which has two segments (I have no idea >> why rust is giving me two segments, but it is). If I boot directly >> into the ELF file from the Pi firmware it boots fine, but if I boot via >> GRUB I have issues with data corruption in .rodata which is in the >> second segment. The first segment (.text) appears to load correctly. > > Could you share the output from "readelf -Wa "? Additionally, > how is your Multiboot2 header defined in the kernel? I don’t have this to hand but I will share when I get back. The multiboot2 header is defined in ASM at the very head of my code. > >> Digging into this, the ELF headers specify a load base address of >> 0x801060 for the second segment, but GRUB allocates and loads this to >> 0x802000. If I change my linker to align the second segment onto >> 0x1000 everything works fine. > > The Multiboot2 protocol does not tolerate "holes" in the image. You can > find good explanation what I mean by that here [1]. I’ll look into this. > >> Is this alignment to 0x1000 a defined behaviour of the GRUB allocator >> or Multiboot2? I'm assuming it's not a problem with the ELF file as >> it's generated by usual means and runs fine otherwise. > > I think you should take closer look at grub-core/loader/multiboot_elfxx.c > and especially lines starting from 258. It seems to me something around > here overwrites part of the image in the memory. > I did this and I now understand what is causing the issue. The memory allocator in grub allocates pages and hence aligns the memory region to 0x1000. When the allocator returns it gives the start of the region allocated and the segment is loaded to that address, not the address specified by the elf file. The workaround I’ve used is to align your second segment to 0x1000 or to only use one segment. Should the defined behaviour be to require segments to be aligned on pages, or should grub load as per the elf file? Chris > Daniel > > [1] https://lists.xenproject.org/archives/html/xen-devel/2021-05/msg01003.html ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: RFC: A partition for grubenv, etc.
On Thu, Sep 23, 2021 at 08:59:14PM -0600, Chris Murphy wrote: > On Wed, Jul 28, 2021 at 6:23 AM Daniel Kiper wrote: > > > > At this point I am not fully convinced the /boot/grub should be on > > a separate filesystem. Though after going through this thread it seems > > to me we should add full write support to the GRUB for a simple file > > system. The FAT looks like a good candidate. Does not it? This way we > > will be able to support writable grubenv without any limitations and have > > flexibility to add other writable files without bigger hassle if needed. > > On UEFI, the firmware's FAT driver should be write capable, but I > don't know if GRUB can make use of it directly rather than using > GRUB's own FAT driver? IIRC it uses own driver. I am not sure at this point we can piggyback on the UEFI FAT driver. If not we have to be sure both drivers, the GRUB and UEFI ones, do not conflict with each other in some ways. > On non-UEFI systems, we still need GRUB to have its own read-write > capable driver. I think parity between BIOS and UEFI is important, but > I'd also say if there's a faster/easier/more maintainable path with > some other file system for other bootloaders, that'd be even more > preferred. I don't know of such a file system. But UDF is in the same > ballpark, long since supported in the kernel, supports optical, > removable, hard drive use cases. We can consider UDF too. Though FAT is much more common and I think we should take it into account. > > Additionally, it is worth mentioning the [1] work which adds a save_env > > redirection layer. It allows us to choose target for grubenv. However, > > if we want make it more generic then we could redirect any writes to any > > target which can be potentially useful. > > Btrfs has two fairly large areas for exclusive bootloader usage, > ~64KiB from partition start to the superblock. And ~1-2 MiB after the > first superblock and before the first block group. But no other file > system I'm aware of has such a large reservation for the bootloader. > XFS has none. ext4 might have 512 bytes. I think it's valid to ask for > an explicit reservation for bootloader usage, but this is just more of > the same issue as grubenv and BIOS Boot. There is no file system in > such a location so in effect it's up to GRUB to internally implement > something, in which case it might as well just have a dedicated > partition for grubfs or whatever it'd be. I think we should use well know simple filesystem to write/store the GRUB data. Coming up with own structure to store the data requires maintaining this custom structure and write e.g. tools which checks and fix their integrity. If we choose filesystem solution we have these tools out of the box and they are maintained by others. So, (potentially) less work for the GRUB folks... :-) > > Last but not least, I think we do not need to change anything in current > > partitions usage. In particular it seems to me the BIOS Boot Partition > > usage should stay as is. > > I don't mind just retasking BIOS Boot into a general "bootloader" > partition that bootloaders can use. Or even make it explicitly a GRUB > bootloader partition type GUID. It's better if each bootloader has its > own? Originally BIOS Boot wasn't "owned" by GRUB, any bootloader can > use it, but insofar as I'm aware it's the only bootloader that uses > this partition type GUID. I think the BIOS boot partition should stay as is and we should introduce separate partition type called e.g. Bootloader data partition. Then it should be formatted as FAT and have relevant directory structure, e.g. the GRUB should store its data in /bootloader/grub directory. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/6] grub-fstest: Fix resource leaks in cmd_cmp()
In the function cmd_cmp() within the while loop, srcnew and destnew are
being allocated but are never freed either before leaving scope or in
the recursive calls being made to cmd_cmp().
Fixes: CID 314032
Fixes: CID 314045
Signed-off-by: Darren Kenny
---
util/grub-fstest.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index 838656420098..da0751222f88 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -299,10 +299,15 @@ cmd_cmp (char *src, char *dest)
*ptr++ = '/';
strcpy (ptr, entry->d_name);
- if (grub_util_is_special_file (destnew))
+ if (grub_util_is_special_file (destnew)) {
+ free(srcnew);
+ free(destnew);
continue;
+ }
cmd_cmp (srcnew, destnew);
+ free(srcnew);
+ free(destnew);
}
grub_util_fd_closedir (dir);
return;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 4/6] grub-mkfont: Fix memory leak in write_font_pf2()
In the function write_font_pf2() memory is allocated for 'font_name' to construct a new name, but it is not released before returning from the function, leaking the allocated memory. Fixes: CID 314015 Signed-off-by: Darren Kenny --- util/grub-mkfont.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/grub-mkfont.c b/util/grub-mkfont.c index 0fe45a6103dd..97e8d27e91d6 100644 --- a/util/grub-mkfont.c +++ b/util/grub-mkfont.c @@ -928,6 +928,7 @@ write_font_pf2 (struct grub_font_info *font_info, char *output_file) file, output_file); } + free(font_name); fclose (file); } -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 6/6] gzio: Fix possible use of uninitialized variable in huft_build()
In huft_build() it is possible to reach the for loop where 'r' is being
assigned to 'q[j]' without 'r.v' ever being initialized.
Fixes: CID 314024
Signed-off-by: Darren Kenny
---
grub-core/io/gzio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/io/gzio.c b/grub-core/io/gzio.c
index aea86a0a9a92..10156e569c85 100644
--- a/grub-core/io/gzio.c
+++ b/grub-core/io/gzio.c
@@ -447,7 +447,7 @@ huft_build (unsigned *b,/* code lengths in bits (all
assumed <= BMAX) */
int l; /* bits per table (returned in m) */
register unsigned *p;/* pointer into c[], b[], or v[] */
register struct huft *q; /* points to current table */
- struct huft r; /* table entry for structure assignment */
+ struct huft r = {0}; /* table entry for structure assignment */
struct huft *u[BMAX];/* table stack */
unsigned v[N_MAX]; /* values in order of bit length */
register int w; /* bits before this table == (l * h) */
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/6] grub-mkrescue: Fix memory leak in write_part()
In the function write_part(), the value of 'inname' is not used beyond the grub_util_fopen() call, so is should be freed to avoid leakage. Fixes: CID 314028 Signed-off-by: Darren Kenny --- util/grub-mkrescue.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c index fb4dcc6d58f7..d5e4c92fd74d 100644 --- a/util/grub-mkrescue.c +++ b/util/grub-mkrescue.c @@ -229,6 +229,7 @@ write_part (FILE *f, const char *srcdir) char *inname = grub_util_path_concat (2, srcdir, "partmap.lst"); char buf[260]; in = grub_util_fopen (inname, "rb"); + free(inname); if (!in) return; while (fgets (buf, 256, in)) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/6] grub-install-common: Fix memory leak in copy_all()
The copy_all() function skips a section of code using continue, but
fails to free the memory in srcf first, leaking it.
Fixes: CID 314026
Signed-off-by: Darren Kenny
---
util/grub-install-common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index 4e212e690c52..0995fa741834 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -753,8 +753,10 @@ copy_all (const char *srcd,
continue;
srcf = grub_util_path_concat (2, srcd, de->d_name);
if (grub_util_is_special_file (srcf)
- || grub_util_is_directory (srcf))
+ || grub_util_is_directory (srcf)) {
+ free(srcf);
continue;
+ }
dstf = grub_util_path_concat (2, dstd, de->d_name);
grub_install_compress_file (srcf, dstf, 1);
free (srcf);
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 5/6] zfs: Fix possible insecure use of chunk size in zap_leaf_array_get()
In zap_leaf_array_get() the chunk size passed in is considered tainted
by Coverity, and is being used before it is tested for validity.
To fix this the assignment of 'la' is moved until after the test of the
value of 'chunk'.
Fixes: CID 314014
Signed-off-by: Darren Kenny
---
grub-core/fs/zfs/zfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index 44e4e18147af..e9d7a7d0e4f6 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -2229,7 +2229,7 @@ zap_leaf_array_get (zap_leaf_phys_t * l,
grub_zfs_endian_t endian, int blksft,
while (bseen < array_len)
{
- struct zap_leaf_array *la = &ZAP_LEAF_CHUNK (l, blksft, chunk)->l_array;
+ struct zap_leaf_array *la;
grub_size_t toread = array_len - bseen;
if (toread > ZAP_LEAF_ARRAY_BYTES)
@@ -2239,6 +2239,7 @@ zap_leaf_array_get (zap_leaf_phys_t * l,
grub_zfs_endian_t endian, int blksft,
/* Don't use grub_error because this error is to be ignored. */
return GRUB_ERR_BAD_FS;
+ la = &ZAP_LEAF_CHUNK (l, blksft, chunk)->l_array;
grub_memcpy (buf + bseen,la->la_array, toread);
chunk = grub_zfs_to_cpu16 (la->la_next, endian);
bseen += toread;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/6] Fix some Coverity low-hanging bugs
Coverity has flagged a number of small issues that should be fixed to help in cleaning up the code - these here are primarily memory leaks or uninitialized variables. In theory leaked memory is significant, but for short-lived processes it is minor. Similarly for unitinialized variables - some compilers will do the right thing and zero out the value allocated on the stack, but some won't. So it is better to be sure of the content that leave it open for possible misuse. Darren Kenny (6): grub-install-common: Fix memory leak in copy_all() grub-mkrescue: Fix memory leak in write_part() grub-fstest: Fix resource leaks in cmd_cmp() grub-mkfont: Fix memory leak in write_font_pf2() zfs: Fix possible insecure use of chunk size in zap_leaf_array_get() gzio: Fix possible use of uninitialized variable in huft_build() grub-core/fs/zfs/zfs.c | 3 ++- grub-core/io/gzio.c| 2 +- util/grub-fstest.c | 7 ++- util/grub-install-common.c | 4 +++- util/grub-mkfont.c | 1 + util/grub-mkrescue.c | 1 + 6 files changed, 14 insertions(+), 4 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] net: Allow use of non-standard TCP/IP ports for HTTP protocol.
Hi Stephen,
On Sun, Oct 10, 2021 at 03:15:34PM -0700, Stephen Balousek wrote:
> Hi Daniel,
>
> Thank you for the review. This is my first submission to the project, and I
> apologize for my lack of familiarity with the conventions and guidelines.
No worries!
> I reworked the patch with your suggested changes, and I also included an
> attempt at some changes for the documentation.
Thanks!
> I am not completely sure how to submit a revised patch to the mailing list
> and still maintain the email thread, so I am appending the new patch to this
> message.
Next time please create a new thread and mark the patch as v2, v3, etc.
respectively.
You can use "git format-patch" and "git send-email" to do it.
[...]
> From 3efc9a7f00a3e8f96609ee95262e87e61d42ce44 Mon Sep 17 00:00:00 2001
> From: Stephen Balousek
> Date: Sun, 10 Oct 2021 15:12:20 -0700
> Subject: [PATCH] http: Allow use of non-standard TCP/IP ports
> To: grub-devel@gnu.org
>
> Allow the use of HTTP servers listening on ports other 80. This is done
> with an extension to the http notation:
>
> (http[,server[,port]])
>
> - or -
>
> (http[,server[:port]])
>
> Signed-off-by: Stephen Balousek
> ---
> docs/grub.texi | 12
> grub-core/net/http.c | 37 +++--
> 2 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 99d0a0149..dbafe80d2 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3011,6 +3011,18 @@ environment variable @samp{net_default_server} is
> used.
> Before using the network drive, you must initialize the network.
> @xref{Network}, for more information.
>
> +For the @samp{http} network protocol, @code{@var{server}} may specify a
> +port number other than the default value of @samp{80}. The server name
> +and port number are separated by either @samp{,} or @samp{:}:
> +
> +@itemize @bullet
> +@item
> +@code{(http,@var{server},@var{port})}
> +
> +@item
> +@code{(http,@var{server}:@var{port})}
> +@end itemize
> +
> If you boot GRUB from a CD-ROM, @samp{(cd)} is available. @xref{Making
> a GRUB bootable CD-ROM}, for details.
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index b616cf40b..eb07e333a 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -312,6 +312,9 @@ http_establish (struct grub_file *file, grub_off_t
> offset, int initial)
> int i;
> struct grub_net_buff *nb;
> grub_err_t err;
> + char *server_name;
> + char *port_string;
> + unsigned long port_number;
>
> nb = grub_netbuff_alloc (GRUB_NET_TCP_RESERVE_SIZE
> + sizeof ("GET ") - 1
> @@ -390,10 +393,40 @@ http_establish (struct grub_file *file, grub_off_t
> offset, int initial)
> grub_netbuff_put (nb, 2);
> grub_memcpy (ptr, "\r\n", 2);
>
> - data->sock = grub_net_tcp_open (file->device->net->server,
> - HTTP_PORT, http_receive,
> + port_string = grub_strrchr (file->device->net->server, ',');
> + if (port_string == NULL)
> + {
> + port_string = grub_strrchr (file->device->net->server, ':');
> + if (port_string != NULL && grub_strchr (port_string + 1, ']') !=
I am not sure why you are looking for ']' here. Could you enlighten me?
> NULL)
> + port_string = NULL;
> + }
> + if (port_string != NULL)
> + {
> + port_number = grub_strtoul (port_string + 1, 0, 10);
> + if (port_number == 0 && grub_errno == GRUB_ERR_BAD_NUMBER)
I think it should be "port_number == GRUB_ULONG_MAX" instead of
"port_number == 0" here.
> + return grub_error (GRUB_ERR_BAD_NUMBER, N_("non-numeric or invalid
> port number `%s'"), port_string + 1);
> + if (port_number == 0 || port_number > 65535)
> + return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("port number `%s' not in
> the range of 1 to 65535"), port_string + 1);
> +
> + server_name = grub_strdup (file->device->net->server);
> + if (server_name == NULL)
> + return grub_errno;
> + server_name[port_string - file->device->net->server] = '\0';
> + }
> + else
> + {
> + port_number = HTTP_PORT;
> + server_name = file->device->net->server;
> + }
Otherwise patch looks much better.
Thanks,
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Print module name on license check failure
Daniel Kiper writes:
> On Mon, Oct 25, 2021 at 06:17:03PM -0400, Robbie Harwood wrote:
>> Before performing the license check, resolve the module name so that it
>> can be printed if the license check fails. Prior to this change, grub
>> would only indicate that the check had been failed, but not by what
>> module. This made it difficult to track down either the problem module,
>> or debug the false positive further.
>>
>> Signed-off-by: Robbie Harwood
>> ---
>> grub-core/kern/dl.c | 21 ++---
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
>> index 48f8a7907..363bacc43 100644
>> --- a/grub-core/kern/dl.c
>> +++ b/grub-core/kern/dl.c
>> @@ -457,14 +457,21 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
>> Be sure to understand your license obligations.
>> */
>> static grub_err_t
>> -grub_dl_check_license (Elf_Ehdr *e)
>> +grub_dl_check_license (grub_dl_t mod, Elf_Ehdr *e)
>> {
>>Elf_Shdr *s = grub_dl_find_section (e, ".module_license");
>> - if (s && (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") == 0
>> -|| grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3+") == 0
>> -|| grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv2+") == 0))
>> + if (!s)
>
> s == NULL please...
>
>> +return grub_error (GRUB_ERR_BAD_MODULE,
>> + "no license section in module %.64s", mod->name);
>> +
>> + if (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") == 0
>> + || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3+") == 0
>> + || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv2+") == 0)
>
> Could use grub_strncmp() instead of grub_strcmp() here?
I don't object to doing so for clarity, but strictly speaking I don't
think it's needed as the LICENSE= strings are terminated.
>> return GRUB_ERR_NONE;
>> - return grub_error (GRUB_ERR_BAD_MODULE, "incompatible license");
>> +
>> + return grub_error (GRUB_ERR_BAD_MODULE,
>> + "incompatible license in module %.64s: %.64s", mod->name,
>> + (char *) e + s->sh_offset);
>> }
>>
>> static grub_err_t
>> @@ -641,8 +648,8 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
>> constitutes linking) and GRUB core being licensed under GPLv3+.
>> Be sure to understand your license obligations.
>>*/
>> - if (grub_dl_check_license (e)
>> - || grub_dl_resolve_name (mod, e)
>> + if (grub_dl_resolve_name (mod, e)
>> + || grub_dl_check_license (mod, e)
>
> I think you should split this patch into two. One which improves error
> messages and another which imposes length restrictions on the
> grub_strcmp() and grub_error() in the grub_dl_check_license().
I can do that, but the NULL-check and strcmp are both pre-existing code
from when the license check was added. Are you sure?
> And I think you should use 63 instead of 64 (63 + NUL gives us 64
> :-)).
Fine.
Be well,
--Robbie
signature.asc
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Retire ChangeLog-2015
Daniel Axtens writes: >> ChangeLog-2015 has been untouched for nearly 5 years now, and any >> information in it is purely for historical purposes. At the same time, >> grepping for code winds up matching this file quite a bit, almost never >> accomplishing anything other than cluttering up your grep results. We >> don't need this in the main repo, and "git show" will find it if you're >> looking at the old history of commits on some file. >> >> This patch deletes it. >> > > Oh goodness yes, what a great idea. Not having to skip past it in git > grep would be wonderful. > > Reviewed-by: Daniel Axtens Reviewed-by: Robbie Harwood Be well, --Robbie signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Print module name on license check failure
On Tue, Oct 26, 2021 at 12:08:23PM -0400, Robbie Harwood wrote:
> Daniel Kiper writes:
>
> > On Mon, Oct 25, 2021 at 06:17:03PM -0400, Robbie Harwood wrote:
> >> Before performing the license check, resolve the module name so that it
> >> can be printed if the license check fails. Prior to this change, grub
> >> would only indicate that the check had been failed, but not by what
> >> module. This made it difficult to track down either the problem module,
> >> or debug the false positive further.
> >>
> >> Signed-off-by: Robbie Harwood
> >> ---
> >> grub-core/kern/dl.c | 21 ++---
> >> 1 file changed, 14 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> >> index 48f8a7907..363bacc43 100644
> >> --- a/grub-core/kern/dl.c
> >> +++ b/grub-core/kern/dl.c
> >> @@ -457,14 +457,21 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
> >> Be sure to understand your license obligations.
> >> */
> >> static grub_err_t
> >> -grub_dl_check_license (Elf_Ehdr *e)
> >> +grub_dl_check_license (grub_dl_t mod, Elf_Ehdr *e)
> >> {
> >>Elf_Shdr *s = grub_dl_find_section (e, ".module_license");
> >> - if (s && (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") == 0
> >> - || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3+") == 0
> >> - || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv2+") == 0))
> >> + if (!s)
> >
> > s == NULL please...
> >
> >> +return grub_error (GRUB_ERR_BAD_MODULE,
> >> + "no license section in module %.64s", mod->name);
> >> +
> >> + if (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") == 0
> >> + || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3+") == 0
> >> + || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv2+") == 0)
> >
> > Could use grub_strncmp() instead of grub_strcmp() here?
>
> I don't object to doing so for clarity, but strictly speaking I don't
> think it's needed as the LICENSE= strings are terminated.
Huh! Yeah, you are right. So, please ignore my stupid comment.
> >> return GRUB_ERR_NONE;
> >> - return grub_error (GRUB_ERR_BAD_MODULE, "incompatible license");
> >> +
> >> + return grub_error (GRUB_ERR_BAD_MODULE,
> >> + "incompatible license in module %.64s: %.64s", mod->name,
> >> + (char *) e + s->sh_offset);
> >> }
> >>
> >> static grub_err_t
> >> @@ -641,8 +648,8 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
> >> constitutes linking) and GRUB core being licensed under GPLv3+.
> >> Be sure to understand your license obligations.
> >>*/
> >> - if (grub_dl_check_license (e)
> >> - || grub_dl_resolve_name (mod, e)
> >> + if (grub_dl_resolve_name (mod, e)
> >> + || grub_dl_check_license (mod, e)
> >
> > I think you should split this patch into two. One which improves error
> > messages and another which imposes length restrictions on the
> > grub_strcmp() and grub_error() in the grub_dl_check_license().
>
> I can do that, but the NULL-check and strcmp are both pre-existing code
> from when the license check was added. Are you sure?
Ditto.
> > And I think you should use 63 instead of 64 (63 + NUL gives us 64
> > :-)).
I will fix it myself before pushing v3.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] net: Allow use of non-standard TCP/IP ports for HTTP protocol.
Hi Daniel,
Thank you for another detailed response. Please let me know how that
section related to IPv6 should be commented, and I will submit an
updated patch.
- Steve
On 10/26/2021 8:52 AM, Daniel Kiper wrote:
Hi Stephen,
On Sun, Oct 10, 2021 at 03:15:34PM -0700, Stephen Balousek wrote:
Hi Daniel,
Thank you for the review. This is my first submission to the project, and I
apologize for my lack of familiarity with the conventions and guidelines.
No worries!
I reworked the patch with your suggested changes, and I also included an
attempt at some changes for the documentation.
Thanks!
I am not completely sure how to submit a revised patch to the mailing list
and still maintain the email thread, so I am appending the new patch to this
message.
Next time please create a new thread and mark the patch as v2, v3, etc.
respectively.
You can use "git format-patch" and "git send-email" to do it.
[...]
From 3efc9a7f00a3e8f96609ee95262e87e61d42ce44 Mon Sep 17 00:00:00 2001
From: Stephen Balousek
Date: Sun, 10 Oct 2021 15:12:20 -0700
Subject: [PATCH] http: Allow use of non-standard TCP/IP ports
To: grub-devel@gnu.org
Allow the use of HTTP servers listening on ports other 80. This is done
with an extension to the http notation:
(http[,server[,port]])
- or -
(http[,server[:port]])
Signed-off-by: Stephen Balousek
---
docs/grub.texi | 12
grub-core/net/http.c | 37 +++--
2 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index 99d0a0149..dbafe80d2 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3011,6 +3011,18 @@ environment variable @samp{net_default_server} is
used.
Before using the network drive, you must initialize the network.
@xref{Network}, for more information.
+For the @samp{http} network protocol, @code{@var{server}} may specify a
+port number other than the default value of @samp{80}. The server name
+and port number are separated by either @samp{,} or @samp{:}:
+
+@itemize @bullet
+@item
+@code{(http,@var{server},@var{port})}
+
+@item
+@code{(http,@var{server}:@var{port})}
+@end itemize
+
If you boot GRUB from a CD-ROM, @samp{(cd)} is available. @xref{Making
a GRUB bootable CD-ROM}, for details.
diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index b616cf40b..eb07e333a 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -312,6 +312,9 @@ http_establish (struct grub_file *file, grub_off_t
offset, int initial)
int i;
struct grub_net_buff *nb;
grub_err_t err;
+ char *server_name;
+ char *port_string;
+ unsigned long port_number;
nb = grub_netbuff_alloc (GRUB_NET_TCP_RESERVE_SIZE
+ sizeof ("GET ") - 1
@@ -390,10 +393,40 @@ http_establish (struct grub_file *file, grub_off_t
offset, int initial)
grub_netbuff_put (nb, 2);
grub_memcpy (ptr, "\r\n", 2);
- data->sock = grub_net_tcp_open (file->device->net->server,
- HTTP_PORT, http_receive,
+ port_string = grub_strrchr (file->device->net->server, ',');
+ if (port_string == NULL)
+ {
+ port_string = grub_strrchr (file->device->net->server, ':');
+ if (port_string != NULL && grub_strchr (port_string + 1, ']') !=
I am not sure why you are looking for ']' here. Could you enlighten me?
IPv6. The idea is to capture the port number '8080' from '[::1]:8080'
but not '1' from '[::1]'. I can add a comment here, since I agree the
intent is not clear.
NULL)
+ port_string = NULL;
+ }
+ if (port_string != NULL)
+ {
+ port_number = grub_strtoul (port_string + 1, 0, 10);
+ if (port_number == 0 && grub_errno == GRUB_ERR_BAD_NUMBER)
I think it should be "port_number == GRUB_ULONG_MAX" instead of
"port_number == 0" here.
Right. I don't know what I was thinking here.
+ return grub_error (GRUB_ERR_BAD_NUMBER, N_("non-numeric or invalid
port number `%s'"), port_string + 1);
+ if (port_number == 0 || port_number > 65535)
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("port number `%s' not in
the range of 1 to 65535"), port_string + 1);
+
+ server_name = grub_strdup (file->device->net->server);
+ if (server_name == NULL)
+ return grub_errno;
+ server_name[port_string - file->device->net->server] = '\0';
+ }
+ else
+ {
+ port_number = HTTP_PORT;
+ server_name = file->device->net->server;
+ }
Otherwise patch looks much better.
Thank you.
Thanks,
Daniel
--
- Steve
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] i386-pc: build btrfs zstd support into separate module
On Tue, Oct 26, 2021 at 02:55:21PM +0200, Daniel Kiper wrote: > On Fri, Sep 10, 2021 at 05:22:22PM +0800, Michael Chang via Grub-devel wrote: > > On Wed, Sep 08, 2021 at 09:37:52PM +0200, Daniel Kiper wrote: > > > On Fri, Sep 03, 2021 at 09:21:39AM +0800, Michael Chang via Grub-devel > > > wrote: > > > > On Thu, Sep 02, 2021 at 02:12:52PM +0200, Daniel Kiper wrote: > > > > > On Thu, Sep 02, 2021 at 01:48:30PM +0800, Michael Chang via > > > > > Grub-devel wrote: > > > > > > On Wed, Sep 01, 2021 at 06:38:22PM +0200, Daniel Kiper wrote: > > > > > > > On Tue, Aug 31, 2021 at 03:12:28PM +0800, Michael Chang via > > > > > > > Grub-devel wrote: > > > > [snip] > > > > > > just that I can't resist to fix problem from our users who opted to use > > > > "btrfs partition" which differs to "short mbr gap" with a lot more user > > > > base and sensible usecases. > > > > > > > > FWIW we want to address this problem, because mbr gap is adjustable via > > > > re-partitioning but btrfs bootloader area is not. > > > > > > Huh! The "btrfs partition" sounds like not resizable MBR gap! I am not > > > happy with it just at the beginning. Anyway, could you explain your use > > > case in more details including example commands and why core.img seems > > > landing in the "btrfs partition". I am especially curious about the > > > latter because I think the "btrfs partition" and things like that was > > > designed for something else, e.g., storing grubenv data. And why this > > > solution is i386 specific? > > > > Installing to btrfs partition is not something exotic to grub, it is > > actually a supported usecase by grub for a long time. Also zfs has > > similar design, see: > > > > c7ba4f698 Support BtrFS embedding. > > ba102053c Support ZFS embedding. > > > > To make it happen, you just have to point the btrfs device to > > grub-install and it will setup and embed image there. That worked quite > > nicely until the zstd support came into play. > > > > On current git head, the command fails with core.img is too large. > > > > # ./grub-install -d ./grub-core /dev/vda2 > > Installing for i386-pc platform. > > ./grub-install: warning: your core.img is unusually large. It won't fit > > in the embedding area. > > ./grub-install: error: filesystem `btrfs' doesn't support blocklists. > > > > With this patch, the embedding still works but warns the user zstd is > > disabled, also instructing them to use MBR if they need the zstd feature > > to work. > > > > # ./grub-install -d ./grub-core /dev/vda2 > > Installing for i386-pc platform. > > ./grub-install: warning: btrfs zstd compression is disabled, please change > > install device to disk. > > Installation finished. No error reported. > > > > It is very i386-pc centric, as it is often used by legacy mbr boot code > > with which active partition is chainloaded. Some people still thinks > > this is the right thing to do and feels more comfortable this way to > > manage their multiple distrubution booting, compared with os-prober. > > I do not like this solution because it is similar to all the patches > trying to solve small MBR problem. Though, after chatting with Btrfs > folks in Oracle, it seems to me there is better solution for this issue. > I think we should make the GRUB installer smarter. If it cannot put the > core image in first 64 KiB before primary superblock it should install > the core image behind the primary superblock, i.e. starting from > 64 KiB + 4 KiB but below 1 MiB. Please take a look at [1] which nicely > explains the Btrfs bootloader support. Does it make sense for you? Yes. The idea is brilliant and would be the sanest solution out there. I'll try to come up with a patch and discuss here. > > Daniel > > [1] > https://btrfs.wiki.kernel.org/index.php/Manpage/btrfs(5)#BOOTLOADER_SUPPORT Thanks for pointing this out. I am looking for it for a long time as I used to come across this paragraph but not being able to make it again. Thanks, Michael > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
