Re: problem in unpack-trees.c

Roman Zippel [off-list ref] writes:

I looked into it and the problem is during the "git-read-tree --reset" 
step and it seems that the local df_conflict_entry variable of 
unpack_trees() survives past that function. If you check in 
add_cache_entry() it's called with this variable and only because 
verify_path() fails it's not added to the tree on the other archs, but on 
m68k the data on the stack is a bit different and thus verify_path() 
succeeds and the stack variable is added to the tree and later saved.

I am very puzzled about this.

You are correct that the address of the df_conflict_entry is
assigned to "struct unpack_trees_options *o" in unpack_trees(),
and add_cache_entry() are called from many places in the call
chain that starts from that function.  And these call sites do
rely on the conflict_entry to have a NUL name to prevent
add_cache_entry from adding the entry to the index.  Which feels
like a hack, but it should get the job done while it is running.

But I do not see the o->df_conflict_entry used after unpack_trees()
returns in git-read-tree.

Using the patch below, you can simulate what's happing on m68k...

The patch does not compile for me (the static definition part),
but it appears that the memset() in question is not clearing the
name field.

Ah....

Oops....

Isn't "struct cache_entry" a structure with a flexible array
member at the end?  I wonder why we have it on the stack to
begin with.

On my x86-64 box with gcc 4 (i.e. "#define FLEX_ARRAY /* empty */"
is used,

        #include "cache.h"

        int
        main(int ac, char **av)
        {
                printf("sz %zu\n", sizeof(struct cache_entry));
                printf("of %zu\n", offsetof(struct cache_entry, name));
                memset(&dfc, 0, sizeof(dfc));
        }

size of "struct cache_entry" is 64 while the offset of name
member is 62, so I am luckily getting two bytes of room for
memset to fill and cause name[] to be properly NUL terminated.
If the alignment requirement of the platform is smaller, we may
be overstepping the struct when we access its name[] member.

Hi,

On Mon, 4 Dec 2006, Junio C Hamano wrote:

Roman Zippel [off-list ref] writes:

quoted

I looked into it and the problem is during the "git-read-tree --reset" 
step and it seems that the local df_conflict_entry variable of 
unpack_trees() survives past that function. If you check in 
add_cache_entry() it's called with this variable and only because 
verify_path() fails it's not added to the tree on the other archs, but on 
m68k the data on the stack is a bit different and thus verify_path() 
succeeds and the stack variable is added to the tree and later saved.

I am very puzzled about this.

You are correct that the address of the df_conflict_entry is
assigned to "struct unpack_trees_options *o" in unpack_trees(),
and add_cache_entry() are called from many places in the call
chain that starts from that function.  And these call sites do
rely on the conflict_entry to have a NUL name to prevent
add_cache_entry from adding the entry to the index.  Which feels
like a hack, but it should get the job done while it is running.

Ok, I see, I wasn't sure that this part was really intentional.

On my x86-64 box with gcc 4 (i.e. "#define FLEX_ARRAY /* empty */"
is used,

        #include "cache.h"

        int
        main(int ac, char **av)
        {
                printf("sz %zu\n", sizeof(struct cache_entry));
                printf("of %zu\n", offsetof(struct cache_entry, name));
                memset(&dfc, 0, sizeof(dfc));
        }

size of "struct cache_entry" is 64 while the offset of name
member is 62, so I am luckily getting two bytes of room for
memset to fill and cause name[] to be properly NUL terminated.
If the alignment requirement of the platform is smaller, we may
be overstepping the struct when we access its name[] member.

Yes, on m68k both values are the same and thus name is not initialized.
Your patch should do the trick, I'll give it a try.