{"thread":{"id":"43261","subject":"Re: problem in unpack-trees.c","startedAt":"2006-12-03T01:21:39Z","lastAt":"2006-12-04T11:36:04Z","messageCount":4,"participants":["Junio C Hamano","Roman Zippel"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"297909","messageId":"Pine.LNX.4.64.0612022125530.1867@scrub.home","threadId":"43261","inReplyTo":null,"subject":"problem in unpack-trees.c","fromName":"Roman Zippel","fromEmail":"zippel@linux-m68k.org","sentAt":"2006-12-03T01:21:39Z","receivedAt":"2006-12-03T01:21:39Z","isPatch":false,"sender":{"key":"zippel@linux-m68k.org","avatar":null},"body":"Hi,\n\nGit currently fails to succeed the tests as you can see here:\nhttp://buildd.debian.org/fetch.cgi?&pkg=git-core&ver=1%3A1.4.4-2&arch=m68k&stamp=1164303729&file=log\n\nI looked into it and the problem is during the \"git-read-tree --reset\" \nstep and it seems that the local df_conflict_entry variable of \nunpack_trees() survives past that function. If you check in \nadd_cache_entry() it's called with this variable and only because \nverify_path() fails it's not added to the tree on the other archs, but on \nm68k the data on the stack is a bit different and thus verify_path() \nsucceeds and the stack variable is added to the tree and later saved.\n\nUsing the patch below, you can simulate what's happing on m68k and now I \nneed some help fixing this properly, as I'm not that familiar with the \ninternals.\nThanks.\n\nbye, Roman\n\ndiff -ur git-core-1.4.4.1/unpack-trees.c git-core-1.4.4.1/unpack-trees.c\n--- git-core-1.4.4.1/unpack-trees.c\t2006-11-23 03:38:07.000000000 +0100\n+++ git-core-1.4.4.1/unpack-trees.c\t2006-12-03 01:54:04.000000000 +0100\n@@ -370,7 +370,10 @@\n \tint i;\n \tstruct object_list *posn = trees;\n \tstruct tree_entry_list df_conflict_list;\n-\tstruct cache_entry df_conflict_entry;\n+\tstatic struct {\n+\t\tstruct cache_entry entry;\n+\t\tchar name[4];\n+\t} df_conflict_entry;\n \n \tmemset(&df_conflict_list, 0, sizeof(df_conflict_list));\n \tdf_conflict_list.next = &df_conflict_list;\n@@ -382,7 +385,8 @@\n \n \to->merge_size = len;\n \tmemset(&df_conflict_entry, 0, sizeof(df_conflict_entry));\n-\to->df_conflict_entry = &df_conflict_entry;\n+\to->df_conflict_entry = &df_conflict_entry.entry;\n+\tdf_conflict_entry.entry.name[0] = 'x';\n \n \tif (len) {\n \t\tposns = xmalloc(len * sizeof(struct tree_entry_list *));\n@@ -399,6 +403,7 @@\n \t\tdie(\"Merge requires file-level merging\");\n \n \tcheck_updates(active_cache, active_nr, o);\n+\tmemset(&df_conflict_entry, 0x11, sizeof(df_conflict_entry));\n \treturn 0;\n }\n"},{"id":"295466","messageId":"7vodqkq956.fsf@assigned-by-dhcp.cox.net","threadId":"43261","inReplyTo":"Pine.LNX.4.64.0612022125530.1867@scrub.home","subject":"Re: problem in unpack-trees.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-04T09:53:57Z","receivedAt":"2006-12-04T09:53:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Roman Zippel <zippel@linux-m68k.org> writes:\n\n> I looked into it and the problem is during the \"git-read-tree --reset\" \n> step and it seems that the local df_conflict_entry variable of \n> unpack_trees() survives past that function. If you check in \n> add_cache_entry() it's called with this variable and only because \n> verify_path() fails it's not added to the tree on the other archs, but on \n> m68k the data on the stack is a bit different and thus verify_path() \n> succeeds and the stack variable is added to the tree and later saved.\n\nI am very puzzled about this.\n\nYou are correct that the address of the df_conflict_entry is\nassigned to \"struct unpack_trees_options *o\" in unpack_trees(),\nand add_cache_entry() are called from many places in the call\nchain that starts from that function.  And these call sites do\nrely on the conflict_entry to have a NUL name to prevent\nadd_cache_entry from adding the entry to the index.  Which feels\nlike a hack, but it should get the job done while it is running.\n\nBut I do not see the o->df_conflict_entry used after unpack_trees()\nreturns in git-read-tree.\n\n> Using the patch below, you can simulate what's happing on m68k...\n\nThe patch does not compile for me (the static definition part),\nbut it appears that the memset() in question is not clearing the\nname field.\n\nAh....\n\nOops....\n\nIsn't \"struct cache_entry\" a structure with a flexible array\nmember at the end?  I wonder why we have it on the stack to\nbegin with.\n\nOn my x86-64 box with gcc 4 (i.e. \"#define FLEX_ARRAY /* empty */\"\nis used,\n\n        #include \"cache.h\"\n\n        int\n        main(int ac, char **av)\n        {\n                printf(\"sz %zu\\n\", sizeof(struct cache_entry));\n                printf(\"of %zu\\n\", offsetof(struct cache_entry, name));\n                memset(&dfc, 0, sizeof(dfc));\n        }\n\nsize of \"struct cache_entry\" is 64 while the offset of name\nmember is 62, so I am luckily getting two bytes of room for\nmemset to fill and cause name[] to be properly NUL terminated.\nIf the alignment requirement of the platform is smaller, we may\nbe overstepping the struct when we access its name[] member.\n\n"},{"id":"294163","messageId":"7vhcwcq8u7.fsf@assigned-by-dhcp.cox.net","threadId":"43261","inReplyTo":"7vodqkq956.fsf@assigned-by-dhcp.cox.net","subject":"Re: problem in unpack-trees.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-04T10:00:32Z","receivedAt":"2006-12-04T10:00:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> If the alignment requirement of the platform is smaller, we may\n> be overstepping the struct when we access its name[] member.\n\nWould this help?\n\nI do not think we need a separate identity for df_conflict_entry\nper invocation of unpack_trees(), so this allocates one such\nentry with NUL name at the first call to the function and reuses\nit from the variable.\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7cfd628..47aa804 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -370,7 +370,7 @@ int unpack_trees(struct object_list *trees, struct unpack_trees_options *o)\n \tint i;\n \tstruct object_list *posn = trees;\n \tstruct tree_entry_list df_conflict_list;\n-\tstruct cache_entry df_conflict_entry;\n+\tstatic struct cache_entry *dfc;\n \n \tmemset(&df_conflict_list, 0, sizeof(df_conflict_list));\n \tdf_conflict_list.next = &df_conflict_list;\n@@ -381,8 +381,10 @@ int unpack_trees(struct object_list *trees, struct unpack_trees_options *o)\n \tstate.refresh_cache = 1;\n \n \to->merge_size = len;\n-\tmemset(&df_conflict_entry, 0, sizeof(df_conflict_entry));\n-\to->df_conflict_entry = &df_conflict_entry;\n+\n+\tif (!dfc)\n+\t\tdfc = xcalloc(1, sizeof(struct cache_entry) + 1);\n+\to->df_conflict_entry = dfc;\n \n \tif (len) {\n \t\tposns = xmalloc(len * sizeof(struct tree_entry_list *));\n"},{"id":"298188","messageId":"Pine.LNX.4.64.0612041214590.1867@scrub.home","threadId":"43261","inReplyTo":"7vodqkq956.fsf@assigned-by-dhcp.cox.net","subject":"Re: problem in unpack-trees.c","fromName":"Roman Zippel","fromEmail":"zippel@linux-m68k.org","sentAt":"2006-12-04T11:36:04Z","receivedAt":"2006-12-04T11:36:04Z","isPatch":false,"sender":{"key":"zippel@linux-m68k.org","avatar":null},"body":"Hi,\n\nOn Mon, 4 Dec 2006, Junio C Hamano wrote:\n\n> Roman Zippel <zippel@linux-m68k.org> writes:\n> \n> > I looked into it and the problem is during the \"git-read-tree --reset\" \n> > step and it seems that the local df_conflict_entry variable of \n> > unpack_trees() survives past that function. If you check in \n> > add_cache_entry() it's called with this variable and only because \n> > verify_path() fails it's not added to the tree on the other archs, but on \n> > m68k the data on the stack is a bit different and thus verify_path() \n> > succeeds and the stack variable is added to the tree and later saved.\n> \n> I am very puzzled about this.\n> \n> You are correct that the address of the df_conflict_entry is\n> assigned to \"struct unpack_trees_options *o\" in unpack_trees(),\n> and add_cache_entry() are called from many places in the call\n> chain that starts from that function.  And these call sites do\n> rely on the conflict_entry to have a NUL name to prevent\n> add_cache_entry from adding the entry to the index.  Which feels\n> like a hack, but it should get the job done while it is running.\n\nOk, I see, I wasn't sure that this part was really intentional.\n\n> On my x86-64 box with gcc 4 (i.e. \"#define FLEX_ARRAY /* empty */\"\n> is used,\n> \n>         #include \"cache.h\"\n> \n>         int\n>         main(int ac, char **av)\n>         {\n>                 printf(\"sz %zu\\n\", sizeof(struct cache_entry));\n>                 printf(\"of %zu\\n\", offsetof(struct cache_entry, name));\n>                 memset(&dfc, 0, sizeof(dfc));\n>         }\n> \n> size of \"struct cache_entry\" is 64 while the offset of name\n> member is 62, so I am luckily getting two bytes of room for\n> memset to fill and cause name[] to be properly NUL terminated.\n> If the alignment requirement of the platform is smaller, we may\n> be overstepping the struct when we access its name[] member.\n\nYes, on m68k both values are the same and thus name is not initialized.\nYour patch should do the trick, I'll give it a try.\n\n"}]}