luci-app-attendedsysupgrade: Fix logic error in EFI image selection #6886

sur5r · 2024-02-03T19:45:40Z

If a non-EFI image comes first in the list of images, it would have been selected even on an EFI system.

Closes: #6884

This PR is not from my main or master branch 💩, but a separate branch ✅
Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
Incremented 🆙 any PKG_VERSION in the Makefile
Tested on: (x86-64, 23.0.5.2, Firefox) ✅
( Preferred ) Mention: @ the original code author for feedback
( Preferred ) Screenshot or mp4 of changes:
( Optional ) Closes: e.g. openwrt/luci#issue-number
Description: (describe the changes proposed in this PR)

systemcrash · 2024-02-04T13:49:18Z

Thank you for this comprehensive PR @sur5r. I thought this was weird because when I was comparing your PR, I had v22 branch checked out. It was identical. So I understood that it had been changed between v22 and master for some important reason.

Please refer to #6623 and d7e905e.

Your fix functionally reverts the above-mentioned commits. ( Meaning if we revert, it once again causes problems a la #6623 ) But for you, it works?

The before and after code seem functionally synonymous:

					if (this.data.efi) {
						if (image.type == 'combined-efi') {
							return image;
						}

vs

					if (this.data.efi && image.type == 'combined-efi') {
						return image;

...

sur5r · 2024-02-04T15:10:38Z

Not, it doesn't revert d7e905e. The problem is very subtle.

The code as currently in master is sensitive to the order of images in the array. It does not reject non-EFI images on EFI systems.

The code before and after is not synonymous as the else part is connected differently.

Please see the following JS code. It contains both the old and the new selectImage function (slightly adapted to work outside a class) and the one from 22.03. It then applies them to the image list from #6884.

images = [
    {
      "filesystem": "ext4",
      "name": "openwrt-23.05.2-afe704423c9b-x86-64-generic-ext4-combined.img.gz",
      "sha256": "a3c2bbb19d48561f95dae77b89f0d8b9b248b38c1dd9182812713393c632d2c0",
      "sha256_unsigned": "37cb2e2122e35a4f5025b15f6d7d207f90feb6addf1649cf5637a5245769c1d6",
      "type": "combined"
    },
    {
      "filesystem": "squashfs",
      "name": "openwrt-23.05.2-afe704423c9b-x86-64-generic-squashfs-combined-efi.img.gz",
      "sha256": "cc51a6393a51c17ef608fb4fdb2cff2c04e0b0e8c8bdb7f3ad607a8ec300836a",
      "sha256_unsigned": "1363ee584ba83972cde8cab83ceee8a0eaa89bc2471d755fdb22ee1d872004c5",
      "type": "combined-efi"
    },
    {
      "filesystem": "squashfs",
      "name": "openwrt-23.05.2-afe704423c9b-x86-64-generic-squashfs-combined.img.gz",
      "sha256": "8cad7d9b8cdac568e5b94fde76ab8607fbecb81e4556cba061493bb8a21636a4",
      "sha256_unsigned": "0164c07ff181050d848bac9478db1f424ea76d70aee1c26c1395308b35effed5",
      "type": "combined"
    },
    {
      "filesystem": "squashfs",
      "name": "openwrt-23.05.2-afe704423c9b-x86-64-generic-squashfs-rootfs.img.gz",
      "sha256": "9fbf34e74d680d769c8b4af748e2585d2328c2586b63d1e53b43f72596f48ff6",
      "sha256_unsigned": "9fbf34e74d680d769c8b4af748e2585d2328c2586b63d1e53b43f72596f48ff6",
      "type": "rootfs"
    },
    {
      "filesystem": "ext4",
      "name": "openwrt-23.05.2-afe704423c9b-x86-64-generic-ext4-combined-efi.img.gz",
      "sha256": "63536ad2d836c7a6885fdd83abc9d5b06168d432b7d6a76c680af79d8312d00f",
      "sha256_unsigned": "7a1068e802164370e3ab4025f5e7d02176a9c842bdbfa6f32c2b48f7715aa637",
      "type": "combined-efi"
    },
    {
      "filesystem": "ext4",
      "name": "openwrt-23.05.2-afe704423c9b-x86-64-generic-ext4-rootfs.img.gz",
      "sha256": "dfd3badb2c8fd95791519ef979332ff427b76ae0b191d64a8001cc7ee2987516",
      "sha256_unsigned": "dfd3badb2c8fd95791519ef979332ff427b76ae0b191d64a8001cc7ee2987516",
      "type": "rootfs"
    }
  ]

firmware = { "filesystem": "ext4", target :"x86/64" }
data = { "efi": "yes" }


selectImage_new=function (images,firmware,data) {
	let image;
	for (image of images) {
		if (firmware.filesystem == image.filesystem) {
			// x86 images can be combined-efi (EFI) or combined (BIOS)
			if(firmware.target.indexOf("x86") != -1) {
				if (data.efi) {
					if (image.type == 'combined-efi') {
						return image;
					}
				} else if (image.type == 'combined') {
					return image;
				}
			} else {
				if (image.type == 'sysupgrade' || image.type == 'combined') {
					return image;
				}
			}
		}
	}
	return null;
}

selectImage_old = function (images,firmware,data) {
	let image;
	for (image of images) {
		if (firmware.filesystem == image.filesystem) {
			// x86 images can be combined-efi (EFI) or combined (BIOS)
			if(firmware.target.indexOf("x86") != -1) {
				if (data.efi && image.type == 'combined-efi') {
					return image;
				} else if (image.type == 'combined') {
					return image;
				}
			} else {
				if (image.type == 'sysupgrade' || image.type == 'combined') {
					return image;
				}
			}
		}
	}
	return null;
}

selectImage_22 = function(images,firmware,data) {
	var image;
	for (image of images) {
		if (firmware.filesystem == image.filesystem) {
			if (data.efi) {
				if (image.type == 'combined-efi') {
					break;
				}
			} else {
				if (image.type == 'sysupgrade' || image.type == 'combined') {
					break;
				}
			}
		}
	}
	return image;
}

console.log("selectImage_new:")
console.log(selectImage_new(images,firmware,data))
console.log("selectImage_old:")
console.log(selectImage_old(images,firmware,data))
console.log("selectImage_22:")
console.log(selectImage_old(images,firmware,data))

Running it shows the behavior as mentioned:

$ js selectImage.js
selectImage_new:
{
  filesystem: 'ext4',
  name: 'openwrt-23.05.2-afe704423c9b-x86-64-generic-ext4-combined-efi.img.gz',
  sha256: '63536ad2d836c7a6885fdd83abc9d5b06168d432b7d6a76c680af79d8312d00f',
  sha256_unsigned: '7a1068e802164370e3ab4025f5e7d02176a9c842bdbfa6f32c2b48f7715aa637',
  type: 'combined-efi'
}
selectImage_old:
{
  filesystem: 'ext4',
  name: 'openwrt-23.05.2-afe704423c9b-x86-64-generic-ext4-combined.img.gz',
  sha256: 'a3c2bbb19d48561f95dae77b89f0d8b9b248b38c1dd9182812713393c632d2c0',
  sha256_unsigned: '37cb2e2122e35a4f5025b15f6d7d207f90feb6addf1649cf5637a5245769c1d6',
  type: 'combined'
}
selectImage_22:
{
  filesystem: 'ext4',
  name: 'openwrt-23.05.2-afe704423c9b-x86-64-generic-ext4-combined.img.gz',
  sha256: 'a3c2bbb19d48561f95dae77b89f0d8b9b248b38c1dd9182812713393c632d2c0',
  sha256_unsigned: '37cb2e2122e35a4f5025b15f6d7d207f90feb6addf1649cf5637a5245769c1d6',
  type: 'combined'
}

sur5r · 2024-02-04T15:38:47Z

Thinking about it, the function seems very fragile. How about switching it to array.filter() instead?

Rough sketch:

selectImage_filter = function(images,firmware,data) {
        fsfilter=function(e) {
                return (e.filesystem == firmware.filesystem);
        }   
        typefilter=function(e) {
                if(firmware.target.indexOf("x86") != -1 && data.efi)
                {   
                        if(data.efi)
                                return (e.type == 'combined-efi')
                        else
                                return (e.type == 'combined');
                }   
                else
                {   
                        return(e.type == 'sysupgrade' || e.type == 'combined');
                }   

        }   
        return images.filter(fsfilter).filter(typefilter)[0]
}

systemcrash · 2024-02-04T15:45:13Z

Thinking about it, the function seems very fragile. How about switching it to array.filter() instead?

I was going to suggest the same thing initially until I thought it best to understand some more details.

Sure, filter. Then we avoid these subtle if clause problems. Perhaps doing away with the if altogether.

sur5r · 2024-02-04T16:04:36Z

I just looked at https://github.com/openwrt/packages/blob/master/utils/auc/src/auc.c#L1653 and find that approach interesting as well. It has a clear preference spelt out as code.

I could write an alternative implementation using filter and easily test it on all 4 x86 variants (MBR/EFI, ext4/squashfs). The only embedded device I could currently test this on is a FritzBOX 7412.

Should I replace it here or open an alternative PR?

systemcrash · 2024-02-04T16:11:05Z

Here is fine. The filter aims to resolve the same bug :) thanks for taking a stab at it. The fritz boxes are all generally arm or ath, which iirc take a sysupgrade

If a non-EFI image comes first in the list of images, it would have been selected even on an EFI system. Signed-off-by: Jakob Haufe <sur5r@sur5r.net>

sur5r · 2024-02-04T20:39:39Z

Rewritten using filter functions.

Tested sucessfully on:

QEMU x86_64 ext4 BIOS
QEMU x86_64 ext4 EFI
QEMU x86_64 squashfs BIOS
QEMU x86_64 squashfs EFI
TP-Link TL-WDR4300 v1 squashfs

aparcar · 2024-02-05T09:05:27Z

Hi, thanks for putting in the work. Change looks fine to me. Let's see how it goes.

Since I'm not a JS developer, please feel free to rework more of the code if that's of your interested, I highly appreciate people taking over this part of ASU.

sur5r · 2024-02-15T14:29:40Z

Unfortunately, I'm not a JS developer as well. I guess it would be great to have auc and luci-app-attendedsysupgrade behave identically, but I understand that this is a lot of work.

sur5r mentioned this pull request Feb 4, 2024

luci-app-attendedsysupgrade: selects MBR image on EFI installation #6884

Closed

luci-app-attendedsysupgrade: Fix logic error in EFI image selection

90e3197

If a non-EFI image comes first in the list of images, it would have been selected even on an EFI system. Signed-off-by: Jakob Haufe <sur5r@sur5r.net>

sur5r force-pushed the asu-efi-fix branch from 9a986e3 to 90e3197 Compare February 4, 2024 20:36

aparcar merged commit cb45737 into openwrt:master Feb 5, 2024
5 checks passed

sur5r deleted the asu-efi-fix branch February 5, 2024 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

luci-app-attendedsysupgrade: Fix logic error in EFI image selection #6886

luci-app-attendedsysupgrade: Fix logic error in EFI image selection #6886

sur5r commented Feb 3, 2024

systemcrash commented Feb 4, 2024

sur5r commented Feb 4, 2024 •

edited

Loading

sur5r commented Feb 4, 2024

systemcrash commented Feb 4, 2024

sur5r commented Feb 4, 2024

systemcrash commented Feb 4, 2024 via email •

edited

Loading

sur5r commented Feb 4, 2024

aparcar commented Feb 5, 2024

sur5r commented Feb 15, 2024

luci-app-attendedsysupgrade: Fix logic error in EFI image selection #6886

luci-app-attendedsysupgrade: Fix logic error in EFI image selection #6886

Conversation

sur5r commented Feb 3, 2024

systemcrash commented Feb 4, 2024

sur5r commented Feb 4, 2024 • edited Loading

sur5r commented Feb 4, 2024

systemcrash commented Feb 4, 2024

sur5r commented Feb 4, 2024

systemcrash commented Feb 4, 2024 via email • edited Loading

sur5r commented Feb 4, 2024

aparcar commented Feb 5, 2024

sur5r commented Feb 15, 2024

sur5r commented Feb 4, 2024 •

edited

Loading

systemcrash commented Feb 4, 2024 via email •

edited

Loading