xorriso in ghostbsd-build

[ASX]

I'm stil wondering why removing a printf() in a different file would change the optimizer behavior ... if it is that the culprit.

[ASX]

Code: Select all

uint16_t *hfsplus_class_pages[256];

static uint16_t class_pages[19][256];

void make_hfsplus_class_pages()
{
    int page_idx = -1, char_idx, i;
    uint16_t *rpt, *page_pt;
    int page_count = 0;

    memset(class_pages, 0, 19 * 256);
    for (i = 0; i < 256; i++)
        hfsplus_class_pages[i] = NULL;

    rpt = (uint16_t *) class_page_data;
    page_pt = (uint16_t *) class_pages;
    while (1) {
        if (*rpt <= page_idx)
    break;
        page_count++;
        page_idx = *(rpt++);
        char_idx = -1;
        while (1) {
            if(*rpt <= char_idx)
        break;
            char_idx = *(rpt++);
            page_pt[char_idx] = *(rpt++);
        }
        rpt++;
        hfsplus_class_pages[page_idx] = class_pages[page_count - 1];
        page_pt += 256;
    }
}

One thing I noticed is the 19 in

Code: Select all

static uint16_t class_pages[19][256];

and when the error happens, it has just executed 19 cycles ... probably going to execute the 20th ...

[scdbackup]

Hi,

you are too fast for me. But i think we caught it now. You by printf,
me by code study and gdb.

make_hfsplus_class_pages();

That's in libisofs/hfsplus_classes.c .

It is suspicious that the last code page block in class_page_data[]
does not end by 0x0, as do all others.
Also the list of 16-bit numbers ends by a comma. As if there was
at least one line missing.

I just ran it under gdb. When the loop ends, the read pointer "rpt" is
two beyond the end of of class_page_data[]:

Code: Select all

  (gdb) p rpt-class_page_data
  $1 = 632
  (gdb) p sizeof(class_page_data)
  $5 = 1262
  (gdb) whatis class_page_data
  type = uint16_t [631]

Clearly a bad memory access. So this is the best suspect we have yet.
The loop does not necessarily have to end at 632 if there is non-zero
stuff read by the last two "rpt" steps.
Option -O0 might have caused some zeros to be stored after the end
of that array, like it happens with gcc.

Now i wonder why valgrind does not raise protest. Grmpf.


I can fix the situation by adding a 16-bit number 0x00 as end marker
of the last page, and one 0x00 as end marker of the list:

Code: Select all

--- libisofs/hfsplus_classes.c  2016-11-13 09:29:36.684112897 +0100
+++ libisofs/hfsplus_classes.c   2017-09-22 15:55:13.680658179 +0200
@@ -422,6 +422,10 @@ static uint16_t class_page_data[] = {
   0x21, 0x230,
   0x22, 0x230,
   0x23, 0x230,
+  0x00,
+
+  /* End of list */
+  0x00
 };
 
 uint16_t *hfsplus_class_pages[256];

Now gdb reports at the end of the loop:

Code: Select all

  (gdb) p rpt-class_page_data
  $14 = 632
  (gdb) p sizeof(class_page_data)
  $15 = 1266
  (gdb) whatis class_page_data
  type = uint16_t [633]

So please try whether this change prevents the crashes.

----------------------------------------------------------------------

I also see an insufficient initialization in libisofs/hfsplus_classes.c :

Code: Select all

static uint16_t class_pages[19][256];

void make_hfsplus_class_pages()
{
    ...
    memset(class_pages, 0, 19 * 256);

Half of the array of 16-bit values stays uninitialized.

But i cannot see how this lapse could lead to a memory fault up to the
end of make_hfsplus_class_pages().

IIRC, static data are initialized automatically by 0.
Nevertheless i shall fix this memset(), too.

----------------------------------------------------------------------

Have a nice day

Thomas

[ASX]

hi scdbackup,

yes, static data should be initialized as zeroes, I agree, so memset() is probably reduntant but won't hurt.

Applyed your patch, to add the end markers, and now it works!

[scdbackup]

Hi,

Applyed you patch, to add the end markers, and now it works!

Yep. The found bug has all properties we were looking for.
I meanwhile assume that -O2 embedded the code of the function into
the code of its only caller. So the printf up there could influence
the behavior of the function.

Did you make sure that all other modifications of xorriso are reverted ?
(It would be embarrassing if your success was just another volatile
effect.)

Have a nice day

Thomas

[ASX]

Did you make sure that all other modifications of xorriso are reverted ?
(It would be embarrassing if your success was just another volatile
effect.)

LoL

Of course yes, I extracted the tarball anew., then applied the patch, (although I edited the file manually ...)

scdbackup, thanks for your fine cooperation on this issue, that afflicted us for long time!

Oh, while we are at it, I noticed a change in license, from GPL 2 to GPL 3, although freebsd ports report 1.4.6 as GPL 2, your tarball show GPL 3 or later.
May be the freeBSD port maintainer need to change that too.

[scdbackup]

Hi,

thanks for your fine cooperation on this issue, that afflicted us for long time!

Thanks to you for hunting down this nasty crack in the code.

I am the one to blame here, not Vladimir Serbinenko. Vladimir submitted
a table as array of C constants which stemmed from a copyrighted example
program.
Because i prefer to keep the number of libisofs copyright holders as
low as possible, i learned from him where the values are specified and
re-narrated those human readable specs as the C table class_page_data[].
Then i wrote the function with the loop which creates the big table
class_pages[] at run time.

Regrettably i damaged my re-narrated table or forgot to complete it.
It is not too bad, i can tell, because it yielded the same class_pages[]
table as found by Vladimir in the example program.
At least as long as gcc was the compiler ...

I noticed a change in license, from GPL 2 to GPL 3

The libraries libisofs, libburn, libisoburn, and the small dynamically
linked xorriso binary from libisoburn are licensed GPLv2+.
This is what you find in most distros.

The all-in-one tarball which you used for the experiments is GNU xorriso,
a friendly and frequently repeated fork of the libraries, which i maintain
for situations like this. It became a GNU project so that GNU GRUB can
depend on it. GNU xorriso is licensed GPLv3+.

The source code of released versions is identical. But the build system
yields differently linked binaries. Just compare the size of both xorrisos
and their need for dynamic libraries.

The libraries exist only as release versions X.Y.even (currently 1.4.8)
and in git. GNU xorriso offers a development snapshot tarball with
version X.Y.odd (currently xorriso-1.4.9.tar.gz) and a script
https://dev.lovelyhq.com/libburnia/libi ... ndalone.sh
by which one can derive it from the libraries' git states.

I committed the fix as

https://dev.lovelyhq.com/libburnia/libi ... ef35b5b2a9
"Bug fix: Reading beyond array end for HFS+ production caused SIGSEGV
with FreeBSD 11 CLANG -O2. Thanks ASX of GhostBSD."

and will also commit the corrected memset() call after some testing.

Have a nice day

Thomas

[scdbackup]

Hi,

to make the license mess complete, the dynamically linked xorriso
updates its license to GPLv3+ when it gets linked with the GPLv3+ licensed
version of libreadline.

Code: Select all

xorriso 1.3.2 : RockRidge filesystem manipulator, libburnia project.
...
Provided under GNU GPL version 3 or later, due to libreadline license.
There is NO WARRANTY, to the extent permitted by law.

One can avoid libreadline by these ./configure options:

Code: Select all

--disable-libreadline --enable-libedit

Installing libedit and its header files before ./configure gives a
convenient dialog command line without xorriso hopping to GPLv3+.


Have a nice day

Thomas

[ASX]

Thomas,

Thanks for clarifying about GPL2/GPL3 licensing.

There is currently no port maintainer for xorriso, in FreeBSD ports, but I was chatting with ericbsd, and very likely he will pickup the maintainership, thus the latest changes will flow in soon.

About the error, well errors just happens, else we would be machines and not humans, there is no need to blame anyone, especially not you because you wrote the code; those who do not write code don't do coding errors, of course.

Again, many thanks!

ASX,
on behalf of GhostBSD Team.

[scdbackup]

Hi,

i uploaded a new GNU xorriso development snapshot tarball with the bug fix:

http://scdbackup.sourceforge.net/xorriso-1.4.9.tar.gz
MD5: 3ff9197427dd13acf0f72c33d1d40287

xorriso -version is supposed to say:

Code: Select all

  xorriso version   :  1.4.9
  Version timestamp :  2017.09.23.074650

The snapshot tarball content and its MD5 can change without change of
the tarball name. Any future version with a newer "Version timestamp :"
is supposed to contain the fix, too.

A new libisofs release will not happen soon. The last one was just a
week ago and the bug is rarely triggered.
I understand from
https://www.freebsd.org/doc/en/books/po ... patch.html
that FreeBSD ports can apply patches.

If it is important to have a libisofs-1.4.8.pl01 release tarball, then one
would need to convince me: [email protected] or [email protected] .

Have a nice day

Thomas