Re: Fwd: Bug#1050663: dar option --on-fly-isolate creates catalogue with broken slice_layout

On 03/09/2023 23:00, Denis Corbin wrote:
> On 03/09/2023 19:37, Rolf Gebhardt wrote:
>> Hello everyone,
> 
> Hi Rolf,

Hi Rolf, Thomas, John, Andrea and all,

I did some code review and step by step debugging without optimization 
today, no definitive conclusion for now, I need some more time to get 
confirmation of the following:

> 
>>
>> I've been following the discussion here and had a look at the patch 
>> and the associated code of the class 'slice_layout'.
>> I noticed one little thing:As far as I can see, the boolean member 
>> variable 'older_sar_than_v8', which acts as condition in the patched 
>> code, is not initialized in the constructor.
> 
> Yes, you are correct, but this is not necessary (read below)

Well, "no necessary" but not clean: this lack of variable initialization 
does not _directly_ seems the cause of the problem, though.

What does a default constructor in C++ seem to do, depend on the C++ 
standard (pivot at C++11) but the object should be zero-initialized, 
that to say, the field slice_layout::older_sar_than_v8 should be set to 
false according to this reference (unless I misread it):

https://en.cppreference.com/w/cpp/language/value_initialization

But you can google and find different assertions... so a clean/robust 
code should explicitly initialize fields, I agree. I will thus fix that 
and review other classes in case I would had made the same wrong assumption.

But assuming this field is garbage (non-initialized), when it comes to 
be used to build the isolated catalogue, the slice_layer::write() method 
is called from header_version::write(), in turn called from 
macro_tools_create_layers() at the beginning of the archive and from 
macro_close_layers() at then end, both in turn called from 
archive::i_archive::op_isolate().

in this slice_layer::write() method, which I reproduce below, whatever 
is the value of older_sar_than_v8, the resulting value assigned to tmp 
is either OLDER_THAN_V8 ('7') or V8 ('8'), one of the two valid values 
expected today. Then, this value get written to archive header/trailer 
at the end of this method:

void slice_layout::write(generic_file & f) const
{
     char tmp = older_sar_than_v8 ? OLDER_THAN_V8 : V8;
     first_size.dump(f);
     other_size.dump(f);
     first_slice_header.dump(f);
     other_slice_header.dump(f);
     f.write(&tmp, 1);
}

If at reading time something else than '7' and '8' is found (triggering 
the 'bug' message) this leads me to think there was something wrong 
about the resulting dar binary, regarding the expected execution.

In addition, the fact Thomas's patch replacing the ternary operator used 
to assign the tmp variable by a classical if/else structure, does fix 
the issue (!), is to me relevant to a problem in gcc, as this should do 
the exact same thing.

So I will clean my backyard and explicitly initialize this field from 
class constructor and test that tomorrow, after I will be able to 
reproduce the bug: But I recreated the Debian-12 VM I had destroyed 
yesterday before the mail of Rolf (email which led me to reconsider 
where was my "duty boundary"). And today gcc-12.2.0 is provided [I think 
it was gcc-12.3.0 yesterday???] the same gcc version as the one provided 
under VoidLinux which does not have any problem with dar. As such I was 
not able to reproduce the bug tonight in order to validate all that...

> 
>>
[...]

Cheers,
Denis