Syslinux - Jun 2014 - [PATCH] utils/isohybrid.c: 007 (2nd try) Enable promised options -u, -m, -b

Hi,

following will be 6 patch proposals for isohybrid.c

1: Encode GPT partition names as UTF-16LE

2: Correct blocking factor in APM partition block counts

3: Correct end block address of first GPT partition

4: Write GPT backup to the very end of the image

5: Change all fseek(3) to fseeko(3)

6: Introduce option --mbr and make isohybrid.c compilable standalone

If the form needs adjustments, i whould be able to reproduce the
patches.


The state after patch 6 was tested by re-hybdridization of
yetserday's Fedora-Live-Desktop-x86_64-20-1.iso.
The result was loaded by xorriso which then assessed and
reported the MBR, GPT, and APM equipment.

One false positive complaint appears:
  GPT backup problems:      Implausible header LBA 1951743, Implausible array
LBA 1951711
It could be avoided by adjusting the ISO filesystem size
to the size of the image file. Regrettably this causes
isohybrid.c to add another MiB of padding if the image
is isohybridized again.
So i will ponder how to recognize the habit of isohybrid.c
as a tolerable deviation - or how to let isohybrid.c recognize
that the filesystem already has a backup GPT at its end,
so that no new space is needed.


Have a nice day :)

Thomas

The worst sin of isohybrid.c was to compose GPT partition names by
8-bit characters and to memcpy() them as if they were 16 bit wide.
GPT names are encoded as UTF-16LE. It is trivial to create this encoding
from 7-bit ASCII.

This change introduces two byte arrays with the desired UTF-16LE names
which replace the string constants "ISOHybrid ISO" and
"ISOHybrid".

--- isohybrid.c.orig	2014-06-22 20:07:19.000000000 +0200
+++ isohybrid.c.001_gpt_name	2014-06-22 20:04:30.000000000 +0200
@@ -749,6 +749,11 @@ initialise_gpt(uint8_t *gpt, uint32_t cu
     struct gpt_part_header *part;
     int hole = 0;
     int gptsize = 128 / 4 + 2;
+    char part_name_iso[] = {'I', 0, 'S', 0, 'O', 0,
'H', 0, 'y', 0,
+                            'b', 0, 'r', 0, 'i', 0,
'd', 0, ' ', 0,
+                            'I', 0, 'S', 0, 'O', 0,  
0, 0};
+    char part_name_bootimg[]= {'I', 0, 'S', 0, 'O', 0,
'H', 0, 'y', 0,
+                               'b', 0, 'r', 0, 'i', 0,
'd', 0,   0, 0};
 
     if (mac_lba) {
 	/* 2048 bytes per partition, plus round to 2048 boundary */
@@ -793,7 +798,7 @@ initialise_gpt(uint8_t *gpt, uint32_t cu
     memcpy(part->partTypeGUID, basic_partition, sizeof(uuid_t));
     part->firstLBA = lendian_64(0);
     part->lastLBA = lendian_64(psize);
-    memcpy(part->name, "ISOHybrid ISO", 28);
+    memcpy(part->name, part_name_iso, 28);
 
     gpt += sizeof(struct gpt_part_header);
     part++;
@@ -802,7 +807,7 @@ initialise_gpt(uint8_t *gpt, uint32_t cu
     memcpy(part->partTypeGUID, basic_partition, sizeof(uuid_t));
     part->firstLBA = lendian_64(efi_lba * 4);
     part->lastLBA = lendian_64(part->firstLBA + efi_count - 1);
-    memcpy(part->name, "ISOHybrid", 20);
+    memcpy(part->name, part_name_bootimg, 20);
 
     gpt += sizeof(struct gpt_part_header);
 
@@ -815,7 +820,7 @@ initialise_gpt(uint8_t *gpt, uint32_t cu
 	memcpy(part->partTypeGUID, hfs_partition, sizeof(uuid_t));
 	part->firstLBA = lendian_64(mac_lba * 4);
 	part->lastLBA = lendian_64(part->firstLBA + mac_count - 1);
-	memcpy(part->name, "ISOHybrid", 20);
+	memcpy(part->name, part_name_bootimg, 20);
 
 	part--;
     }

The block counts in the APM partitions assumed 512 bytes per block,
whereas the start block numbers assume 2048 as announced in the APM header.

This change divides the affected block counts by 4 to correct the assumption.

--- isohybrid.c.001_gpt_name	2014-06-22 20:04:30.000000000 +0200
+++ isohybrid.c.002_apm_block_count	2014-06-22 20:05:13.000000000 +0200
@@ -842,7 +842,7 @@ initialise_apm(uint8_t *gpt, uint32_t st
     part->signature = bendian_short(0x504d);
     part->map_count = bendian_int(apm_parts);
     part->start_block = bendian_int(1);
-    part->block_count = bendian_int(0x10);
+    part->block_count = bendian_int(4);
     strcpy(part->name, "Apple");
     strcpy(part->type, "Apple_partition_map");
     part->data_start = bendian_int(0);
@@ -854,11 +854,11 @@ initialise_apm(uint8_t *gpt, uint32_t st
     part->signature = bendian_short(0x504d);
     part->map_count = bendian_int(3);
     part->start_block = bendian_int(efi_lba);
-    part->block_count = bendian_int(efi_count);
+    part->block_count = bendian_int(efi_count / 4);
     strcpy(part->name, "EFI");
     strcpy(part->type, "Apple_HFS");
     part->data_start = bendian_int(0);
-    part->data_count = bendian_int(efi_count);
+    part->data_count = bendian_int(efi_count / 4);
     part->status = bendian_int(0x33);
 
     part = (struct apple_part_header *)(gpt + 4096);
@@ -868,11 +868,11 @@ initialise_apm(uint8_t *gpt, uint32_t st
 	part->signature = bendian_short(0x504d);
 	part->map_count = bendian_int(3);
 	part->start_block = bendian_int(mac_lba);
-	part->block_count = bendian_int(mac_count);
+	part->block_count = bendian_int(mac_count / 4);
 	strcpy(part->name, "EFI");
 	strcpy(part->type, "Apple_HFS");
 	part->data_start = bendian_int(0);
-	part->data_count = bendian_int(mac_count);
+	part->data_count = bendian_int(mac_count / 4);
 	part->status = bendian_int(0x33);
     } else {
 	part->signature = bendian_short(0x504d);

The GPT partition 1 covers the whole ISO filesystem size. GPT specs
demand that the partition end block number shall be the last valid
block in the partition. isohybrid.c rather wrote the number of the
first block after the partition end.

This change reduces the number by 1.

--- isohybrid.c.002_apm_block_count	2014-06-22 20:05:13.000000000 +0200
+++ isohybrid.c.003_gpt_part_1_last_lba	2014-06-22 20:05:03.000000000 +0200
@@ -797,7 +797,7 @@ initialise_gpt(uint8_t *gpt, uint32_t cu
     memcpy(part->partGUID, iso_uuid, sizeof(uuid_t));
     memcpy(part->partTypeGUID, basic_partition, sizeof(uuid_t));
     part->firstLBA = lendian_64(0);
-    part->lastLBA = lendian_64(psize);
+    part->lastLBA = lendian_64(psize - 1);
     memcpy(part->name, part_name_iso, 28);
 
     gpt += sizeof(struct gpt_part_header);

The GPT backup header block should start 512 bytes before the end of the
image file (resp. end of the disk device). This block and the backup GPT
array were wrongly written 512 bytes too early.

This change brings the backup GPT at its correct position. 

--- isohybrid.c.003_gpt_part_1_last_lba	2014-06-22 20:05:03.000000000 +0200
+++ isohybrid.c.004_gpt_backup_adr	2014-06-22 20:09:06.000000000 +0200
@@ -1084,7 +1084,7 @@ main(int argc, char *argv[])
 	 * Primary GPT starts at sector 1, secondary GPT starts at 1 sector
 	 * before the end of the image
 	 */
-	initialise_gpt(buf, 1, (isostat.st_size + padding - 1024) / 512, 1);
+	initialise_gpt(buf, 1, (isostat.st_size + padding - 512) / 512, 1);
 
 	if (fseek(fp, 512, SEEK_SET))
 	    err(1, "%s: seek error - 6", argv[0]);
@@ -1122,7 +1122,7 @@ main(int argc, char *argv[])
 
 	buf += orig_gpt_size - sizeof(struct gpt_header);
 
-	initialise_gpt(buf, (isostat.st_size + padding - 1024) / 512, 1, 0);
+	initialise_gpt(buf, (isostat.st_size + padding - 512) / 512, 1, 0);
 
 	/* Shift back far enough to write the 128 GPT entries */
 	buf -= 128 * sizeof(struct gpt_part_header);
@@ -1132,7 +1132,7 @@ main(int argc, char *argv[])
 	 * end of the image
 	 */
 
-	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size - 512,
+	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size,
 		  SEEK_SET))
 	    err(1, "%s: seek error - 8", argv[0]);

It seems unwise to offer future programmers fseek(3) calls for copy+paste.
They are simply insufficient for large image files.

This change switches all calls of fseek(3) to fseeko(3) and takes care
that the offset value if of type off_t.

--- isohybrid.c.004_gpt_backup_adr	2014-06-22 20:09:06.000000000 +0200
+++ isohybrid.c.005_fseeko	2014-06-22 20:09:01.000000000 +0200
@@ -916,13 +916,13 @@ main(int argc, char *argv[])
     if (!(fp = fopen(argv[0], "r+")))
         err(1, "could not open file `%s'", argv[0]);
 
-    if (fseek(fp, (16 << 11), SEEK_SET))
+    if (fseeko(fp, (off_t) (16 << 11), SEEK_SET))
         err(1, "%s: seek error - 0", argv[0]);
 
     if (fread(&descriptor, sizeof(char), sizeof(descriptor), fp) !=
sizeof(descriptor))
         err(1, "%s: read error - 0", argv[0]);
 
-    if (fseek(fp, 17 * 2048, SEEK_SET))
+    if (fseeko(fp, (off_t) 17 * 2048, SEEK_SET))
         err(1, "%s: seek error - 1", argv[0]);
 
     bufz = buf = calloc(BUFSIZE, sizeof(char));
@@ -935,7 +935,7 @@ main(int argc, char *argv[])
     if (mode & VERBOSE)
         printf("catalogue offset: %d\n", catoffset);
 
-    if (fseek(fp, catoffset * 2048, SEEK_SET))
+    if (fseeko(fp, ((off_t) catoffset) * 2048, SEEK_SET))
         err(1, "%s: seek error - 2", argv[0]);
 
     buf = bufz;
@@ -985,7 +985,7 @@ main(int argc, char *argv[])
 	}
     }
 
-    if (fseek(fp, (de_lba * 2048 + 0x40), SEEK_SET))
+    if (fseeko(fp, (((off_t) de_lba) * 2048 + 0x40), SEEK_SET))
         err(1, "%s: seek error - 3", argv[0]);
 
     buf = bufz;
@@ -1021,7 +1021,7 @@ main(int argc, char *argv[])
 
     if (!id)
     {
-        if (fseek(fp, 440, SEEK_SET))
+        if (fseeko(fp, (off_t) 440, SEEK_SET))
             err(1, "%s: seek error - 4", argv[0]);
 
 	if (fread(&id, 1, 4, fp) != 4)
@@ -1045,7 +1045,7 @@ main(int argc, char *argv[])
     if (mode & VERBOSE)
         display_mbr(buf, i);
 
-    if (fseek(fp, 0, SEEK_SET))
+    if (fseeko(fp, (off_t) 0, SEEK_SET))
         err(1, "%s: seek error - 5", argv[0]);
 
     if (fwrite(buf, sizeof(char), i, fp) != (size_t)i)
@@ -1086,7 +1086,7 @@ main(int argc, char *argv[])
 	 */
 	initialise_gpt(buf, 1, (isostat.st_size + padding - 512) / 512, 1);
 
-	if (fseek(fp, 512, SEEK_SET))
+	if (fseeko(fp, (off_t) 512, SEEK_SET))
 	    err(1, "%s: seek error - 6", argv[0]);
 
 	if (fwrite(buf, sizeof(char), gpt_size, fp) != (size_t)gpt_size)
@@ -1103,7 +1103,7 @@ main(int argc, char *argv[])
 
 	initialise_apm(buf, APM_OFFSET);
 
-	fseek(fp, APM_OFFSET, SEEK_SET);
+	fseeko(fp, (off_t) APM_OFFSET, SEEK_SET);
 	fwrite(buf, sizeof(char), apm_size, fp);
     }
 
@@ -1132,8 +1132,7 @@ main(int argc, char *argv[])
 	 * end of the image
 	 */
 
-	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size,
-		  SEEK_SET))
+	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size, SEEK_SET))
 	    err(1, "%s: seek error - 8", argv[0]);
 
 	if (fwrite(buf, sizeof(char), orig_gpt_size, fp) != orig_gpt_size)

Although isohybrid.c is supposed to be a companion of the local SYSLINUX
installation, there may be situations where the file isolinux.bin and the
matching MBR template do not stem directly from such an installation.

This change adds an option --mbr, which allows to load an MBR template
file. This may be an isohdp[fp]x*.bin MBR template from the local SYSLINUX
installation, or the first 512 bytes of the isohybrid-capable ISO image
from which isolinux.bin and the other ISOLINUX files are taken.

If macro ISOHYBRID_C_STANDALONE is defined, then the hardcoded MBR templates
are not accessible and isohdpfx.o is not needed at compile time. In this
case, option --mbr becomes mandatory.
I used this for testing my changes with Fedora-Live-Desktop-x86_64-20-1.iso.

isohybrid.c is then compilable without further components of ISOLINUX by:

  cc -DISOHYBRID_C_STANDALONE -Wall -o isohybrid isohybrid.c -luuid

Test run:

  cp Fedora-Live-Desktop-x86_64-20-1.iso \
     Fedora-Live-Desktop-x86_64-20-1-rehybrid.iso

  valgrind ./isohybrid --uefi --mac \
           --mbr Fedora-Live-Desktop-x86_64-20-1.mbr \
           Fedora-Live-Desktop-x86_64-20-1-rehybrid.iso

yields:

  ==13828== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 1)
  ...
  ==13828== LEAK SUMMARY:
  ==13828==    definitely lost: 2,048 bytes in 1 blocks.

(Not that valgrind would have detected the memcpy() abuse of patch 001.
 But at least i seem to have not introduced more obvious sins.)

--- isohybrid.c.005_fseeko	2014-06-22 20:09:01.000000000 +0200
+++ isohybrid.c.006_opt_mbr_standalone	2014-06-22 20:10:49.000000000 +0200
@@ -63,6 +63,8 @@ uint32_t id = 0;                /* MBR: 
 uint8_t hd0 = 0;                /* 0 <= hd0 <= 2 */
 uint8_t partok = 0;             /* 0 <= partok <= 1 */
 
+char mbr_template_path[1024] = {0};   /* Path to MBR template */
+
 uint16_t ve[16];
 uint32_t catoffset = 0;
 uint32_t c = 0, cc = 0, cs = 0;
@@ -223,7 +225,7 @@ usage(void)
 void
 printh(void)
 {
-#define FMT "%-18s %s\n"
+#define FMT "%-20s %s\n"
 
     usage();
 
@@ -237,6 +239,7 @@ printh(void)
     printf(FMT, "   -i --id", "Specify MBR ID (default
random)");
     printf(FMT, "   -u --uefi", "Build EFI bootable
image");
     printf(FMT, "   -m --mac", "Add AFP table support");
+    printf(FMT, "   -b --mbr <PATH>", "Load MBR from
PATH");
 
     printf("\n");
     printf(FMT, "   --forcehd0", "Assume we are loaded as disk
ID 0");
@@ -272,6 +275,7 @@ check_option(int argc, char *argv[])
         { "partok", no_argument, NULL, 'p'},
 	{ "uefi", no_argument, NULL, 'u'},
 	{ "mac", no_argument, NULL, 'm'},
+        { "mbr", required_argument, NULL, 'b' },
 
         { "help", no_argument, NULL, '?' },
         { "verbose", no_argument, NULL, 'v' },
@@ -347,6 +351,12 @@ check_option(int argc, char *argv[])
 		errx(1, "setting an entry is unsupported with EFI or Mac");
 	    break;
 
+	case 'b':
+            if (strlen(optarg) >= sizeof(mbr_template_path))
+                errx(1, "--mbr : Path too long");
+            strcpy(mbr_template_path, optarg);
+            break;
+
         case 'v':
             mode |= VERBOSE;
             break;
@@ -571,6 +581,24 @@ display_catalogue(void)
     printf("de_mbz2: %hu\n", de_mbz2);
 }
 
+
+void
+read_mbr_template(char *path, uint8_t *mbr)
+{
+    FILE *fp;
+    int ret;
+
+    fp = fopen(path, "rb");
+    if (fp == NULL)
+        err(1, "could not open MBR template file `%s'", path);
+    clearerr(fp);
+    ret = fread(mbr, 1, MBRSIZE, fp);
+    if (ferror(fp))
+        err(1, "error while reading MBR template file `%s'",
path);
+    fclose(fp);
+}
+
+
 int
 initialise_mbr(uint8_t *mbr)
 {
@@ -580,9 +608,25 @@ initialise_mbr(uint8_t *mbr)
     uint8_t bhead = 0, bsect = 0, bcyle = 0;
     uint8_t ehead = 0, esect = 0, ecyle = 0;
 
+#ifndef ISOHYBRID_C_STANDALONE
     extern unsigned char isohdpfx[][MBRSIZE];
+#endif
+
+    if (mbr_template_path[0]) {
+        read_mbr_template(mbr_template_path, mbr);
+    } else {
+
+#ifdef ISOHYBRID_C_STANDALONE
+
+        err(1, "This is a standalone binary. You must specify --mbr. E.g
with /usr/lib/syslinux/isohdpfx.bin");
 
-    memcpy(mbr, &isohdpfx[hd0 + 3 * partok], MBRSIZE);
+#else
+
+        memcpy(mbr, &isohdpfx[hd0 + 3 * partok], MBRSIZE);
+
+#endif /* ! ISOHYBRID_C_STANDALONE */
+
+    }
 
     if (mode & MAC) {
 	memcpy(mbr, afp_header, sizeof(afp_header));

It turned out that i copied the options handling from suboptimal
example code.

This change enables the options -u, -m, -b as promised by the help text.

--- isohybrid.c.006_opt_mbr_standalone	2014-06-22 20:10:49.000000000 +0200
+++ isohybrid.c.007_short_opts_umb	2014-06-22 22:14:03.000000000 +0200
@@ -262,7 +262,7 @@ check_option(int argc, char *argv[])
     char *err = NULL;
     int n = 0, ind = 0;
 
-    const char optstr[] = ":h:s:e:o:t:i:fcp?vV";
+    const char optstr[] = ":h:s:e:o:t:i:u:m:b:fcp?vV";
     struct option lopt[] = \
     {
         { "entry", required_argument, NULL, 'e' },

The worst sin of isohybrid.c was to compose GPT partition names by
8-bit characters and to memcpy() them as if they were 16 bit wide.
GPT names are encoded as UTF-16LE. It is trivial to create this encoding
from 7-bit ASCII.

This change introduces two byte arrays with the desired UTF-16LE names
which replace the string constants "ISOHybrid ISO" and
"ISOHybrid".
---
 utils/isohybrid.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/utils/isohybrid.c b/utils/isohybrid.c
index 05afd29..c5b4281 100644
--- a/utils/isohybrid.c
+++ b/utils/isohybrid.c
@@ -749,6 +749,11 @@ initialise_gpt(uint8_t *gpt, uint32_t current, uint32_t
alternate, int primary)
     struct gpt_part_header *part;
     int hole = 0;
     int gptsize = 128 / 4 + 2;
+    char part_name_iso[] = {'I', 0, 'S', 0, 'O', 0,
'H', 0, 'y', 0,
+                            'b', 0, 'r', 0, 'i', 0,
'd', 0, ' ', 0,
+                            'I', 0, 'S', 0, 'O', 0,  
0, 0};
+    char part_name_bootimg[]= {'I', 0, 'S', 0, 'O', 0,
'H', 0, 'y', 0,
+                               'b', 0, 'r', 0, 'i', 0,
'd', 0,   0, 0};
 
     if (mac_lba) {
 	/* 2048 bytes per partition, plus round to 2048 boundary */
@@ -793,7 +798,7 @@ initialise_gpt(uint8_t *gpt, uint32_t current, uint32_t
alternate, int primary)
     memcpy(part->partTypeGUID, basic_partition, sizeof(uuid_t));
     part->firstLBA = lendian_64(0);
     part->lastLBA = lendian_64(psize);
-    memcpy(part->name, "ISOHybrid ISO", 28);
+    memcpy(part->name, part_name_iso, 28);
 
     gpt += sizeof(struct gpt_part_header);
     part++;
@@ -802,7 +807,7 @@ initialise_gpt(uint8_t *gpt, uint32_t current, uint32_t
alternate, int primary)
     memcpy(part->partTypeGUID, basic_partition, sizeof(uuid_t));
     part->firstLBA = lendian_64(efi_lba * 4);
     part->lastLBA = lendian_64(part->firstLBA + efi_count - 1);
-    memcpy(part->name, "ISOHybrid", 20);
+    memcpy(part->name, part_name_bootimg, 20);
 
     gpt += sizeof(struct gpt_part_header);
 
@@ -815,7 +820,7 @@ initialise_gpt(uint8_t *gpt, uint32_t current, uint32_t
alternate, int primary)
 	memcpy(part->partTypeGUID, hfs_partition, sizeof(uuid_t));
 	part->firstLBA = lendian_64(mac_lba * 4);
 	part->lastLBA = lendian_64(part->firstLBA + mac_count - 1);
-	memcpy(part->name, "ISOHybrid", 20);
+	memcpy(part->name, part_name_bootimg, 20);
 
 	part--;
     }
-- 
1.8.4.2

The block counts in the APM partitions assumed 512 bytes per block,
whereas the start block numbers assume 2048 as announced in the APM header.

This change divides the affected block counts by 4 to correct the assumption.
---
 utils/isohybrid.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/utils/isohybrid.c b/utils/isohybrid.c
index c5b4281..7d0864e 100644
--- a/utils/isohybrid.c
+++ b/utils/isohybrid.c
@@ -842,7 +842,7 @@ initialise_apm(uint8_t *gpt, uint32_t start)
     part->signature = bendian_short(0x504d);
     part->map_count = bendian_int(apm_parts);
     part->start_block = bendian_int(1);
-    part->block_count = bendian_int(0x10);
+    part->block_count = bendian_int(4);
     strcpy(part->name, "Apple");
     strcpy(part->type, "Apple_partition_map");
     part->data_start = bendian_int(0);
@@ -854,11 +854,11 @@ initialise_apm(uint8_t *gpt, uint32_t start)
     part->signature = bendian_short(0x504d);
     part->map_count = bendian_int(3);
     part->start_block = bendian_int(efi_lba);
-    part->block_count = bendian_int(efi_count);
+    part->block_count = bendian_int(efi_count / 4);
     strcpy(part->name, "EFI");
     strcpy(part->type, "Apple_HFS");
     part->data_start = bendian_int(0);
-    part->data_count = bendian_int(efi_count);
+    part->data_count = bendian_int(efi_count / 4);
     part->status = bendian_int(0x33);
 
     part = (struct apple_part_header *)(gpt + 4096);
@@ -868,11 +868,11 @@ initialise_apm(uint8_t *gpt, uint32_t start)
 	part->signature = bendian_short(0x504d);
 	part->map_count = bendian_int(3);
 	part->start_block = bendian_int(mac_lba);
-	part->block_count = bendian_int(mac_count);
+	part->block_count = bendian_int(mac_count / 4);
 	strcpy(part->name, "EFI");
 	strcpy(part->type, "Apple_HFS");
 	part->data_start = bendian_int(0);
-	part->data_count = bendian_int(mac_count);
+	part->data_count = bendian_int(mac_count / 4);
 	part->status = bendian_int(0x33);
     } else {
 	part->signature = bendian_short(0x504d);
-- 
1.8.4.2

The GPT partition 1 covers the whole ISO filesystem size. GPT specs
demand that the partition end block number shall be the last valid
block in the partition. isohybrid.c rather wrote the number of the
first block after the partition end.

This change reduces the number by 1.
---
 utils/isohybrid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/isohybrid.c b/utils/isohybrid.c
index 7d0864e..ff6b930 100644
--- a/utils/isohybrid.c
+++ b/utils/isohybrid.c
@@ -797,7 +797,7 @@ initialise_gpt(uint8_t *gpt, uint32_t current, uint32_t
alternate, int primary)
     memcpy(part->partGUID, iso_uuid, sizeof(uuid_t));
     memcpy(part->partTypeGUID, basic_partition, sizeof(uuid_t));
     part->firstLBA = lendian_64(0);
-    part->lastLBA = lendian_64(psize);
+    part->lastLBA = lendian_64(psize - 1);
     memcpy(part->name, part_name_iso, 28);
 
     gpt += sizeof(struct gpt_part_header);
-- 
1.8.4.2

The GPT backup header block should start 512 bytes before the end of the
image file (resp. end of the disk device). This block and the backup GPT
array were wrongly written 512 bytes too early.

This change brings the backup GPT at its correct position.
---
 utils/isohybrid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/utils/isohybrid.c b/utils/isohybrid.c
index ff6b930..1c8f0b6 100644
--- a/utils/isohybrid.c
+++ b/utils/isohybrid.c
@@ -1084,7 +1084,7 @@ main(int argc, char *argv[])
 	 * Primary GPT starts at sector 1, secondary GPT starts at 1 sector
 	 * before the end of the image
 	 */
-	initialise_gpt(buf, 1, (isostat.st_size + padding - 1024) / 512, 1);
+	initialise_gpt(buf, 1, (isostat.st_size + padding - 512) / 512, 1);
 
 	if (fseek(fp, 512, SEEK_SET))
 	    err(1, "%s: seek error - 6", argv[0]);
@@ -1122,7 +1122,7 @@ main(int argc, char *argv[])
 
 	buf += orig_gpt_size - sizeof(struct gpt_header);
 
-	initialise_gpt(buf, (isostat.st_size + padding - 1024) / 512, 1, 0);
+	initialise_gpt(buf, (isostat.st_size + padding - 512) / 512, 1, 0);
 
 	/* Shift back far enough to write the 128 GPT entries */
 	buf -= 128 * sizeof(struct gpt_part_header);
@@ -1132,7 +1132,7 @@ main(int argc, char *argv[])
 	 * end of the image
 	 */
 
-	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size - 512,
+	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size,
 		  SEEK_SET))
 	    err(1, "%s: seek error - 8", argv[0]);
 
-- 
1.8.4.2

It seems unwise to offer future programmers fseek(3) calls for copy+paste.
They are simply insufficient for large image files.

This change switches all calls of fseek(3) to fseeko(3) and takes care
that the offset value if of type off_t.
---
 utils/isohybrid.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/utils/isohybrid.c b/utils/isohybrid.c
index 1c8f0b6..072402f 100644
--- a/utils/isohybrid.c
+++ b/utils/isohybrid.c
@@ -916,13 +916,13 @@ main(int argc, char *argv[])
     if (!(fp = fopen(argv[0], "r+")))
         err(1, "could not open file `%s'", argv[0]);
 
-    if (fseek(fp, (16 << 11), SEEK_SET))
+    if (fseeko(fp, (off_t) (16 << 11), SEEK_SET))
         err(1, "%s: seek error - 0", argv[0]);
 
     if (fread(&descriptor, sizeof(char), sizeof(descriptor), fp) !=
sizeof(descriptor))
         err(1, "%s: read error - 0", argv[0]);
 
-    if (fseek(fp, 17 * 2048, SEEK_SET))
+    if (fseeko(fp, (off_t) 17 * 2048, SEEK_SET))
         err(1, "%s: seek error - 1", argv[0]);
 
     bufz = buf = calloc(BUFSIZE, sizeof(char));
@@ -935,7 +935,7 @@ main(int argc, char *argv[])
     if (mode & VERBOSE)
         printf("catalogue offset: %d\n", catoffset);
 
-    if (fseek(fp, catoffset * 2048, SEEK_SET))
+    if (fseeko(fp, ((off_t) catoffset) * 2048, SEEK_SET))
         err(1, "%s: seek error - 2", argv[0]);
 
     buf = bufz;
@@ -985,7 +985,7 @@ main(int argc, char *argv[])
 	}
     }
 
-    if (fseek(fp, (de_lba * 2048 + 0x40), SEEK_SET))
+    if (fseeko(fp, (((off_t) de_lba) * 2048 + 0x40), SEEK_SET))
         err(1, "%s: seek error - 3", argv[0]);
 
     buf = bufz;
@@ -1021,7 +1021,7 @@ main(int argc, char *argv[])
 
     if (!id)
     {
-        if (fseek(fp, 440, SEEK_SET))
+        if (fseeko(fp, (off_t) 440, SEEK_SET))
             err(1, "%s: seek error - 4", argv[0]);
 
 	if (fread(&id, 1, 4, fp) != 4)
@@ -1045,7 +1045,7 @@ main(int argc, char *argv[])
     if (mode & VERBOSE)
         display_mbr(buf, i);
 
-    if (fseek(fp, 0, SEEK_SET))
+    if (fseeko(fp, (off_t) 0, SEEK_SET))
         err(1, "%s: seek error - 5", argv[0]);
 
     if (fwrite(buf, sizeof(char), i, fp) != (size_t)i)
@@ -1086,7 +1086,7 @@ main(int argc, char *argv[])
 	 */
 	initialise_gpt(buf, 1, (isostat.st_size + padding - 512) / 512, 1);
 
-	if (fseek(fp, 512, SEEK_SET))
+	if (fseeko(fp, (off_t) 512, SEEK_SET))
 	    err(1, "%s: seek error - 6", argv[0]);
 
 	if (fwrite(buf, sizeof(char), gpt_size, fp) != (size_t)gpt_size)
@@ -1103,7 +1103,7 @@ main(int argc, char *argv[])
 
 	initialise_apm(buf, APM_OFFSET);
 
-	fseek(fp, APM_OFFSET, SEEK_SET);
+	fseeko(fp, (off_t) APM_OFFSET, SEEK_SET);
 	fwrite(buf, sizeof(char), apm_size, fp);
     }
 
@@ -1132,8 +1132,7 @@ main(int argc, char *argv[])
 	 * end of the image
 	 */
 
-	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size,
-		  SEEK_SET))
+	if (fseeko(fp, (isostat.st_size + padding) - orig_gpt_size, SEEK_SET))
 	    err(1, "%s: seek error - 8", argv[0]);
 
 	if (fwrite(buf, sizeof(char), orig_gpt_size, fp) != orig_gpt_size)
-- 
1.8.4.2

Although isohybrid.c is supposed to be a companion of the local SYSLINUX
installation, there may be situations where the file isolinux.bin and the
matching MBR template do not stem directly from such an installation.

This change adds an option --mbr, which allows to load an MBR template
file. This may be an isohdp[fp]x*.bin MBR template from the local SYSLINUX
installation, or the first 512 bytes of the isohybrid-capable ISO image
from which isolinux.bin and the other ISOLINUX files are taken.

If macro ISOHYBRID_C_STANDALONE is defined, then the hardcoded MBR templates
are not accessible and isohdpfx.o is not needed at compile time. In this
case, option --mbr becomes mandatory.
I used this for testing my changes with Fedora-Live-Desktop-x86_64-20-1.iso.

isohybrid.c is then compilable without further components of ISOLINUX by:

  cc -DISOHYBRID_C_STANDALONE -Wall -o isohybrid isohybrid.c -luuid

Test run:

  cp Fedora-Live-Desktop-x86_64-20-1.iso \
     Fedora-Live-Desktop-x86_64-20-1-rehybrid.iso

  valgrind ./isohybrid --uefi --mac \
           --mbr Fedora-Live-Desktop-x86_64-20-1.mbr \
           Fedora-Live-Desktop-x86_64-20-1-rehybrid.iso

yields:

  ==13828== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 1)
  ...
  ==13828== LEAK SUMMARY:
  ==13828==    definitely lost: 2,048 bytes in 1 blocks.

(Not that valgrind would have detected the memcpy() abuse of patch 001.
 But at least i seem to have not introduced more obvious sins.)
---
 utils/isohybrid.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/utils/isohybrid.c b/utils/isohybrid.c
index 072402f..0011b78 100644
--- a/utils/isohybrid.c
+++ b/utils/isohybrid.c
@@ -63,6 +63,8 @@ uint32_t id = 0;                /* MBR: 0 <= id <=
0xFFFFFFFF(4294967296) */
 uint8_t hd0 = 0;                /* 0 <= hd0 <= 2 */
 uint8_t partok = 0;             /* 0 <= partok <= 1 */
 
+char mbr_template_path[1024] = {0};   /* Path to MBR template */
+
 uint16_t ve[16];
 uint32_t catoffset = 0;
 uint32_t c = 0, cc = 0, cs = 0;
@@ -223,7 +225,7 @@ usage(void)
 void
 printh(void)
 {
-#define FMT "%-18s %s\n"
+#define FMT "%-20s %s\n"
 
     usage();
 
@@ -237,6 +239,7 @@ printh(void)
     printf(FMT, "   -i --id", "Specify MBR ID (default
random)");
     printf(FMT, "   -u --uefi", "Build EFI bootable
image");
     printf(FMT, "   -m --mac", "Add AFP table support");
+    printf(FMT, "   -b --mbr <PATH>", "Load MBR from
PATH");
 
     printf("\n");
     printf(FMT, "   --forcehd0", "Assume we are loaded as disk
ID 0");
@@ -272,6 +275,7 @@ check_option(int argc, char *argv[])
         { "partok", no_argument, NULL, 'p'},
 	{ "uefi", no_argument, NULL, 'u'},
 	{ "mac", no_argument, NULL, 'm'},
+        { "mbr", required_argument, NULL, 'b' },
 
         { "help", no_argument, NULL, '?' },
         { "verbose", no_argument, NULL, 'v' },
@@ -347,6 +351,12 @@ check_option(int argc, char *argv[])
 		errx(1, "setting an entry is unsupported with EFI or Mac");
 	    break;
 
+	case 'b':
+            if (strlen(optarg) >= sizeof(mbr_template_path))
+                errx(1, "--mbr : Path too long");
+            strcpy(mbr_template_path, optarg);
+            break;
+
         case 'v':
             mode |= VERBOSE;
             break;
@@ -571,6 +581,24 @@ display_catalogue(void)
     printf("de_mbz2: %hu\n", de_mbz2);
 }
 
+
+void
+read_mbr_template(char *path, uint8_t *mbr)
+{
+    FILE *fp;
+    int ret;
+
+    fp = fopen(path, "rb");
+    if (fp == NULL)
+        err(1, "could not open MBR template file `%s'", path);
+    clearerr(fp);
+    ret = fread(mbr, 1, MBRSIZE, fp);
+    if (ferror(fp))
+        err(1, "error while reading MBR template file `%s'",
path);
+    fclose(fp);
+}
+
+
 int
 initialise_mbr(uint8_t *mbr)
 {
@@ -580,9 +608,25 @@ initialise_mbr(uint8_t *mbr)
     uint8_t bhead = 0, bsect = 0, bcyle = 0;
     uint8_t ehead = 0, esect = 0, ecyle = 0;
 
+#ifndef ISOHYBRID_C_STANDALONE
     extern unsigned char isohdpfx[][MBRSIZE];
+#endif
+
+    if (mbr_template_path[0]) {
+        read_mbr_template(mbr_template_path, mbr);
+    } else {
+
+#ifdef ISOHYBRID_C_STANDALONE
+
+        err(1, "This is a standalone binary. You must specify --mbr. E.g
with /usr/lib/syslinux/isohdpfx.bin");
 
-    memcpy(mbr, &isohdpfx[hd0 + 3 * partok], MBRSIZE);
+#else
+
+        memcpy(mbr, &isohdpfx[hd0 + 3 * partok], MBRSIZE);
+
+#endif /* ! ISOHYBRID_C_STANDALONE */
+
+    }
 
     if (mode & MAC) {
 	memcpy(mbr, afp_header, sizeof(afp_header));
-- 
1.8.4.2

Op 2014-06-22 om 21:02 schreef Thomas Schmitt:
> Hi,
> 
> following will be 6 patch proposals for isohybrid.c
> 
> 1: Encode GPT partition names as UTF-16LE
> 
> 2: Correct blocking factor in APM partition block counts
> 
> 3: Correct end block address of first GPT partition
> 
> 4: Write GPT backup to the very end of the image
> 
> 5: Change all fseek(3) to fseeko(3)
> 
> 6: Introduce option --mbr and make isohybrid.c compilable standalone
> 
> If the form needs adjustments, i whould be able to reproduce the
> patches.
Those patches regenerated from git are send to Thomas and the ML.
We will see is they go a cross various e-mail (spam) filters.

> The state after patch 6 was tested by re-hybdridization of
> yetserday's Fedora-Live-Desktop-x86_64-20-1.iso.
> The result was loaded by xorriso which then assessed and
> reported the MBR, GPT, and APM equipment.
> 
> One false positive complaint appears:
>   GPT backup problems:      Implausible header LBA 1951743, Implausible
array LBA 1951711
> It could be avoided by adjusting the ISO filesystem size
> to the size of the image file. Regrettably this causes
> isohybrid.c to add another MiB of padding if the image
> is isohybridized again.
> So i will ponder how to recognize the habit of isohybrid.c
> as a tolerable deviation - or how to let isohybrid.c recognize
> that the filesystem already has a backup GPT at its end,
> so that no new space is needed.
Please do.

> Have a nice day :)
> 
> Thomas

Groeten
Geert Stappers
-- 
Leven en laten leven

Op 2014-06-22 om 22:31 schreef Geert Stappers:
> Op 2014-06-22 om 21:02 schreef Thomas Schmitt:
> > 
> > following will be 6 patch proposals for isohybrid.c
> > 
> > 1: Encode GPT partition names as UTF-16LE
> > 2: Correct blocking factor in APM partition block counts
> > 3: Correct end block address of first GPT partition
> > 4: Write GPT backup to the very end of the image
> > 5: Change all fseek(3) to fseeko(3)
> > 6: Introduce option --mbr and make isohybrid.c compilable standalone
> > 
> > If the form needs adjustments, i whould be able to reproduce the
> > patches.
> 
> Those patches regenerated from git are send to Thomas and the ML.
> We will see is they go a cross various e-mail (spam) filters.
  We will see if they go a cross various e-mail (spam) filters.

They did all six.

There was a seventh patch.
It uses also reformatting.
Don't wait for me do it.


Groeten
Geert Stappers
-- 
Leven en laten leven

My first attempt ro enable the short options for --uefi, --mac, --mbr
was faulty. Here is the second try.

This change enables the options -u, -m, -b as promised by the help text.

--- isohybrid.c.006_opt_mbr_standalone	2014-06-22 20:10:49.000000000 +0200
+++ isohybrid.c.007_short_opts_umb	2014-06-23 19:52:59.000000000 +0200
@@ -262,7 +262,7 @@ check_option(int argc, char *argv[])
     char *err = NULL;
     int n = 0, ind = 0;
 
-    const char optstr[] = ":h:s:e:o:t:i:fcp?vV";
+    const char optstr[] = ":h:s:e:o:t:i:b:umfcp?vV";
     struct option lopt[] = \
     {
         { "entry", required_argument, NULL, 'e' },