Syslinux - Apr 2017 - Patch: Fix isohybrid.c tool under Windows platforms

> Hi,
> 
> Colin Finck wrote:
> > Precisely, if isohybrid currently finds a 0x0A (LF) byte in the MBR
code
> > (which exists in the strings), it adds a 0x0D (CR) before, thereby
> > shifting the MBR bytes to the right and making it invalid.
> > This can be fixed by opening the file in binary mode with the
"b" flag.
> 
> This is a very reasonable proposal.
> If one wants the same behavior of fread(3) and fwrite(3) on descendants of
> Unix and DOS, then one should in any case add "b" to the fopen(3)
mode
> string.
> 
> Not only MBR writing but also reading of the Primary Volume Descriptor
> and of the El Torito catalog by isohybrid.c are currently prone to
> unwanted CR-insertion. Miscalculations can be caused by this.
 
 
Is there anything that can be done in order to reduce such chances of 
miscalculation and/or unwanted insertion of (CR) characters?

> 
> The fopen() call for reading external MBR code (option -m) already has
"b".
> 
> The old perl code contains a call to function binmode, which does about
> the same as C fopen(3) with "b" in the mode string.

So, is there any reason not to apply the suggested patch? Any potential 
compatibility issues? Backward compatibility between different versions 
of isohybrid and isolinux.bin? Anything?

> 
> 
> Have a nice day :)
> 
> Thomas
> 
Regards,
Ady.

Hi,

i wrote:
> > Not only MBR writing but also reading of the Primary Volume Descriptor
> > and of the El Torito catalog by isohybrid.c are currently prone to
> > unwanted CR-insertion. Miscalculations can be caused by this.
I have to correct myself: Carriage Return characters (byte value 13 = 0x0d)
are not at risk to be inserted, but rather to be removed if they happen
to be followed by a New Line character (value 10 = 0x0a).

So for example the block address bytes 0d 0a 00 00 (= 2573 decimal) of the
BIOS boot image in the default entry of the boot catalog would be crippled
during fread() in
  http://git.zytor.com/syslinux/syslinux.git/tree/utils/isohybrid.c#n997 
and then misinterpreted in function read_catalogue() when variable de_lba
gets set:
  http://git.zytor.com/syslinux/syslinux.git/tree/utils/isohybrid.c#n519
de_lba is the value which gets written to byte 432 ff. in the isohybrid MBR.

During boot the MBR would hopefully see a wrong signature and refuse with
  "isolinux.bin missing or corrupt.\r\n"
rather than executing the bytes at the wrong address as program code
(which would be quite scary).


Ady wrote:
> Is there anything that can be done in order to reduce such chances of 
> miscalculation and/or unwanted insertion of (CR) characters?
The appropriate remedy is to use "b" with fopen().

Its absence in isohybrid.c is an obvious translation error which happened
when the perl lines at
  http://git.zytor.com/syslinux/syslinux.git/tree/utils/isohybrid.in#n133

  open(FILE, "+< $file\0") or die "$0: cannot open $file:
$!\n";
  binmode FILE;

were converted to the C lines at
  http://git.zytor.com/syslinux/syslinux.git/tree/utils/isohybrid.c#n970

      if (!(fp = fopen(argv[0], "r+")))
          err(1, "could not open file `%s'", argv[0]);

> So, is there any reason not to apply the suggested patch? Any potential 
> compatibility issues? Backward compatibility between different versions 
> of isohybrid and isolinux.bin? Anything?
No. It's just a bug fix.
The bug has years of tradition. But it is very hard to imagine any use of
its more or less random alterations of bytes which happen only on DOS
descendants but not on Unix.
 

Have a nice day :)

Thomas

Am 17.04.2017 um 15:44 schrieb Thomas Schmitt via
Syslinux:
>> So, is there any reason not to apply the suggested patch? Any potential
>> compatibility issues? Backward compatibility between different versions
>> of isohybrid and isolinux.bin? Anything?
> 
> No. It's just a bug fix.
So, is anything more going to happen now?
My patch is apparently accepted, yet nobody is pushing it to the
repository...


- Colin