Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Rugged tree lookup fails with IndexerError 'Failed to parse tree. Can't parse filemode' #123

Closed
nickh opened this Issue · 12 comments

5 participants

@nickh

In tracking down a haystack needle I found a case where Rugged (using version 0.17.0b7) throws an exception attempting to lookup a tree that seems to exist, while having no problems with a lookup on a similar tree that seems to have the same filemode:

>> c
=> #<Rugged::Commit:2175487900 {message: "Lista de exercicios\n", tree: #<Rugged::Tree:2175413500 {oid: af4f454f39ccf00fbd3aedad4eef6bec486a54f4}>
  <".gitignore" 5bb5fb96f41cb363d29f684742a989a19a47f84b>
  <"README.txt" 8474d965011aba57aca368884671aa4c7f8d352f>
  <"SistemaAutoria" 0810fb7818088ff5ac41ee49199b51473b1bd6c7>
  <"SistemaAutoria_UI" b80bcf5e6d420a596b0bd57324929ca90db0ebc0>
  <"SistemaInterpretacao" 4e74669a70ed37fc47f21db6b842844822d7b413>
, parents: be31e81643a75a9d024eb5ecf2a1d656bff90830>
>> c.tree[2]
=> {:type=>:tree, :filemode=>16384, :oid=>"0810fb7818088ff5ac41ee49199b51473b1bd6c7", :name=>"SistemaAutoria"}
>> c.tree[3]
=> {:type=>:tree, :filemode=>16384, :oid=>"b80bcf5e6d420a596b0bd57324929ca90db0ebc0", :name=>"SistemaAutoria_UI"}
>> rug.lookup c.tree[2][:oid]
Rugged::IndexerError: Failed to parse tree. Can't parse filemode
    from /Users/github/dev/slumlord/vendor/gems/ruby/1.8/gems/rugged-0.17.0.b7/lib/rugged/repository.rb:47:in `lookup'
    from /Users/github/dev/slumlord/vendor/gems/ruby/1.8/gems/rugged-0.17.0.b7/lib/rugged/repository.rb:47:in `lookup'
    from (irb):19
>> rug.lookup c.tree[3][:oid]
=> #<Rugged::Tree:2175383700 {oid: b80bcf5e6d420a596b0bd57324929ca90db0ebc0}>
  <".classpath" 5c26027fcb34c30390eb3b7d75931e01b9208e98>
  <".project" fb56db7c5d4a060d7c4e36662f08ff2bdcb6e746>
  <".settings" 8980f3cac083576ed5182edf61bdb9ed7e019c24>
  <"English.xml" 44e10caececa143e868635355496e74f094f4679>
  <"Lista de Progamação Linguagem C.xml" 2edc4a5302c3a7ef1dd1df05ae8cb6c44a2d9e75>
  <"Portugues.xml" d1fc623bf1c5c5485f751e5f9437fd5f5359f762>
  <"User.xml" 2404191fff4ba2c4d9904146c9fcc4911df67202>
  <"beansbinding-1.2.1.jar" c33b81edd37f9f5190279e9a19041d4adec412dc>
  <"imagens" 39537d4e68e59ccb618942d5ee7b84d726498d9a>
  <"src" c06b81e9380a337bf8a13c18e6e0f4d39d8ca588>
  <"test" dc437fdc4d7fa3272b46b657e5002392fdad9e80>

Using the git command line to look up the problematic tree seems to work fine:

github@nictocat ~/dev/slummin_repos/BrOrlandi/LECA.git> git show 0810fb7818088ff5ac41ee49199b51473b1bd6c7
tree 0810fb7818088ff5ac41ee49199b51473b1bd6c7

.classpath
.project
.settings/
ListaC.xml
ListaTeste.xml
QuestionTest.xml
User.xml
src/
test/
xstream-1.4.2.jar
@nickh

@carlosmn nailed it, there's an illegal mode on the ListaTeste.xml blob in that tree:

github@nictocat ~/dev/slummin_repos/BrOrlandi/LECA.git> git ls-tree 0810f
100644 blob ea1af46202d2497570db8c29b01eb2ffa4551a1c    .classpath
100644 blob 16c70c998e4bb21feeda39ef421aaedda7f25f80    .project
040000 tree 8980f3cac083576ed5182edf61bdb9ed7e019c24    .settings
100644 blob e748c436210635b2bca1598eb326cbf9b7ca9f38    ListaC.xml
100600 blob bfaeba71f5e7043a82c0f230e697470f42c5927d    ListaTeste.xml
100644 blob 792990f9280704b37a4c5b48d35323049a31e56c    QuestionTest.xml
100644 blob 49822122ab53b4e4745e26640e51591c884338d7    User.xml
040000 tree dd8df623a7df0ea947435da639d29917d26804fa    src
040000 tree d4b62a39b6645ff8a8afa1e199f91098db2eb803    test
100644 blob fa8e2ed50bbe06b54d1d55993659b4ed423a8448    xstream-1.4.2.jar
@carlosmn
Owner

It turns out that 0810fb7818088ff5ac41ee49199b51473b1bd6c7 contains an invalid entry

100600 blob bfaeba71f5e7043a82c0f230e697470f42c5927d    ListaTeste.xml

@vmg should we add this to the whitelist? Something like

diff --git a/src/tree.c b/src/tree.c
index 6f98388..8bc5244 100644
--- a/src/tree.c
+++ b/src/tree.c
@@ -20,6 +20,7 @@ static bool valid_filemode(const int filemode)
                || filemode == GIT_FILEMODE_BLOB
                || filemode == GIT_FILEMODE_BLOB_GROUP_WRITABLE
                || filemode == GIT_FILEMODE_BLOB_EXECUTABLE
+               || filemode == GIT_FILEMODE_BLOB_GROUP_NONE
                || filemode == GIT_FILEMODE_LINK
                || filemode == GIT_FILEMODE_COMMIT);
 }
diff --git a/src/tree.h b/src/tree.h
index b67c552..4be6f23 100644
--- a/src/tree.h
+++ b/src/tree.h
@@ -58,4 +58,9 @@ int git_tree__write_index(git_oid *oid, git_index *index, git_repository *repo);
  */
 #define GIT_FILEMODE_BLOB_GROUP_WRITABLE 0100664

+/**
+ * This has been spotted in the wild
+ */
+#define GIT_FILEMODE_BLOB_GROUP_NONE 0100600
+
 #endif
@arrbee
Owner

I feel like instead of going this route, we should separate the two use cases of valid_filemode - be tolerant in what we find and strict in what we use for creating trees. In tree_parse_buffer could we just check that filemode & 0000777 is non zero if (filemode & 0777000) == 0100000. On the other hand, inside git_treebuilder_insert, I think we should use the existing check (and actually remove the GIT_FILEMODE_BLOB_GROUP_WRITABLE option) for creating new trees. Ultimately, if we introduce a warnings API, this is a case where we would warn about the input, but it doesn't seem like a case where we should abort the operation.

@nulltoken
Collaborator

be tolerant in what we find and strict in what we use for creating trees

:+1:

and actually remove the GIT_FILEMODE_BLOB_GROUP_WRITABLE option

I think that this mode is already dealt with, accordingly to the rule above. Some tests covering this behavior @ tests-clar/object/tree/attributes.c.

@carlosmn
Owner

It's dealt with by treating it as valid, which is what arrbee is trying to avoid. Rightly so, as it's not valid, simply "acceptable" for reading in. Making sure we only consider valid the ones that we should write is definitely good. The lax rule should save us from having to special-case every broken creator out there.

@carlosmn
Owner

In a somewhat unrelated issue. Why is rugged raising a IndexerError?

@nulltoken
Collaborator

be tolerant in what we find and strict in what we use for creating trees

it's not valid, simply "acceptable"

Does this mean that the user would be forbidden to create a treebuilder from a tree containing an invalid mode?

@arrbee
Owner

Does this mean that the user would be forbidden to create a treebuilder from a tree containing an invalid mode?

Interesting. I was thinking about git_treebuilder_insert() which should certainly reject (or normalize) invalid modes. It seems like there are three options for git_treebuilder_create() in this situation:

  1. Accept the invalid mode if it is part of an existing tree
  2. Return an error that the tree has an invalid mode
  3. Silently convert the mode to a valid mode using the core git mode-scrubbing logic (i.e. goodmode = (mode & 0100) ? GIT_FILEMODE_BLOB_EXECUTABLE : GIT_FILEMODE_BLOB)

Personally, I'm in favor of option 3 -- when you create a tree in libgit2, we should normalize blob modes according the the above logic. But I'm open to discussion...

By the way, if/when we add a "warning / logging" mechanism into libgit2, this is the type of thing where I'd like to issue a warning message ("normalizing mode 0100600 to 0100644 for blob 'your/file.txt'") but still complete the operation successfully.

@nulltoken
Collaborator

Personally, I'm in favor of option 3 -- when you create a tree in libgit2, we should normalize blob modes according the the above logic. But I'm open to discussion...

:+1:

@carlosmn carlosmn referenced this issue in libgit2/libgit2
Merged

tree: relax the filemode parser #1123

@arrbee
Owner

I believe this issue is fixed via libgit2/libgit2@f1c75b9

Is there any way to confirm and close the bug?

@nickh

Confirmed:

irb(main):013:0> c
=> #<Rugged::Commit:2217450100 {message: "Lista de exercicios\n", tree: #<Rugged::Tree:2217406380 {oid: af4f454f39ccf00fbd3aedad4eef6bec486a54f4}>
  <".gitignore" 5bb5fb96f41cb363d29f684742a989a19a47f84b>
  <"README.txt" 8474d965011aba57aca368884671aa4c7f8d352f>
  <"SistemaAutoria" 0810fb7818088ff5ac41ee49199b51473b1bd6c7>
  <"SistemaAutoria_UI" b80bcf5e6d420a596b0bd57324929ca90db0ebc0>
  <"SistemaInterpretacao" 4e74669a70ed37fc47f21db6b842844822d7b413>
, parents: be31e81643a75a9d024eb5ecf2a1d656bff90830>
irb(main):014:0> c.tree[2]
=> {:type=>:tree, :oid=>"0810fb7818088ff5ac41ee49199b51473b1bd6c7", :filemode=>16384, :name=>"SistemaAutoria"}
irb(main):015:0> c.tree[3]
=> {:type=>:tree, :oid=>"b80bcf5e6d420a596b0bd57324929ca90db0ebc0", :filemode=>16384, :name=>"SistemaAutoria_UI"}
irb(main):016:0> rug.lookup c.tree[2][:oid]
=> #<Rugged::Tree:2217379860 {oid: 0810fb7818088ff5ac41ee49199b51473b1bd6c7}>
  <".classpath" ea1af46202d2497570db8c29b01eb2ffa4551a1c>
  <".project" 16c70c998e4bb21feeda39ef421aaedda7f25f80>
  <".settings" 8980f3cac083576ed5182edf61bdb9ed7e019c24>
  <"ListaC.xml" e748c436210635b2bca1598eb326cbf9b7ca9f38>
  <"ListaTeste.xml" bfaeba71f5e7043a82c0f230e697470f42c5927d>
  <"QuestionTest.xml" 792990f9280704b37a4c5b48d35323049a31e56c>
  <"User.xml" 49822122ab53b4e4745e26640e51591c884338d7>
  <"src" dd8df623a7df0ea947435da639d29917d26804fa>
  <"test" d4b62a39b6645ff8a8afa1e199f91098db2eb803>
  <"xstream-1.4.2.jar" fa8e2ed50bbe06b54d1d55993659b4ed423a8448>

irb(main):017:0> rug.lookup c.tree[3][:oid]
=> #<Rugged::Tree:2217367300 {oid: b80bcf5e6d420a596b0bd57324929ca90db0ebc0}>
  <".classpath" 5c26027fcb34c30390eb3b7d75931e01b9208e98>
  <".project" fb56db7c5d4a060d7c4e36662f08ff2bdcb6e746>
  <".settings" 8980f3cac083576ed5182edf61bdb9ed7e019c24>
  <"English.xml" 44e10caececa143e868635355496e74f094f4679>
  <"Lista de Progamação Linguagem C.xml" 2edc4a5302c3a7ef1dd1df05ae8cb6c44a2d9e75>
  <"Portugues.xml" d1fc623bf1c5c5485f751e5f9437fd5f5359f762>
  <"User.xml" 2404191fff4ba2c4d9904146c9fcc4911df67202>
  <"beansbinding-1.2.1.jar" c33b81edd37f9f5190279e9a19041d4adec412dc>
  <"imagens" 39537d4e68e59ccb618942d5ee7b84d726498d9a>
  <"src" c06b81e9380a337bf8a13c18e6e0f4d39d8ca588>
  <"test" dc437fdc4d7fa3272b46b657e5002392fdad9e80>

In particular, it looks like the blob mode is being fixed:

irb(main):061:0> t2['ListaTeste.xml']
=> {:type=>:blob, :oid=>"bfaeba71f5e7043a82c0f230e697470f42c5927d", :filemode=>33188, :name=>"ListaTeste.xml"}
irb(main):062:0> '%06o' % 33188
=> "100644"
@vmg
Owner

PARTY ON.

@vmg vmg closed this
@carlosmn carlosmn referenced this issue from a commit in carlosmn/rugged
@nickh nickh Bump version and libgit2 submodule to the latest
Want the fix for libgit2#123
9f77aa2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.