Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Tree#count_recursive #464

Merged
merged 16 commits into from
Mar 9, 2015
Merged

Add Tree#count_recursive #464

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b6ab69f
Add Tree#count_recursive
Mar 4, 2015
4f629c0
Kill debugging
Mar 4, 2015
1b636a9
hard tabs... seriously...
Mar 4, 2015
62d6952
Count won't be negative
Mar 4, 2015
0933e8f
Cast to the right type
Mar 4, 2015
8589139
Allow setting a limit on Tree#count_recursive
Mar 5, 2015
e1b7acf
Add docs for limit param
Mar 5, 2015
2c70da0
Count and limit should have the same max value
Mar 5, 2015
000d0da
Limit can be negative, just make both count and limit signed, 2.1 bil…
Mar 5, 2015
759dcfd
Just use Check_Type rather than a manual TYPE() + rb_raise
Mar 5, 2015
272d487
Better variable name conventions
Mar 6, 2015
c31d47d
No need to default to nil with rb_scan_args
Mar 6, 2015
5d3c4da
Move Tree#count_recursive payload struct into rugged_tree.c
Mar 6, 2015
44117e0
Add braces for if statements
Mar 9, 2015
8a765f8
Whitespace
Mar 9, 2015
501f69d
Declaration before statements
Mar 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions ext/rugged/rugged_tree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,68 @@ static VALUE rb_git_tree_entrycount(VALUE self)
return INT2FIX(git_tree_entrycount(tree));
}

struct rugged_treecount_cb_payload
{
int count;
int limit;
};

static int rugged__treecount_cb(const char *root, const git_tree_entry *entry, void *data)
{
struct rugged_treecount_cb_payload *payload = data;

if (payload->limit >= 0 && payload->count >= payload->limit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add braces here? I know we generally don't use braces for single-line bodies, but I feel this looks a bit weird because of the last else condition which spans multiple lines.

return -1;
else if(git_tree_entry_type(entry) == GIT_OBJ_TREE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Add a whitespace after if.

return 0;
else {
++(payload->count);
return 1;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately in the libgit2 git_tree_walk API, there is no maximum depth check and the implementation is recursive, so there are some theoretical DOS vectors here. I think probably the best bet is to fix it at the libgit2 level and either reimplement git_tree_walk using the git_iterator_for_tree API or simply add a depth check internal to the API. (It would be nice to reflect the depth as a parameter to the callback, but that would be a breaking API change, so probably not worth it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point. Rather than just counting blob, I could count all tree entries (including subtrees). This means that providing a limit will give protection from at least one more DOS vector. Would that cover the case you're thinking of?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regrettably, I think not. The issue is actually hard to compensate for except at the libgit2 level (without introducing a waste of time scanning path data) - namely that you can create a 100,000 level deep tree and not hit your walk limit, but burn up a huge amount of stack space internal to libgit2. Let me look over there a bit...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw MRI cannot stack overflow because it protects the end of the stack in the VM and will gracefully raise a StackOverflow exception into Ruby-land if a C extension overflows... So I'm not sure I'd consider this an attack vector.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you @vmg for bringing actual knowledge to the table. 😽


/*
* call-seq:
* tree.count_recursive(limit=nil) -> count
*
* `limit` - The maximum number of blobs to the count in the repository.
* Rugged will stop walking the tree after `limit` items to avoid long
* execution times.
*
* Return the number of blobs (up to the limit) contained in the tree and
* all subtrees.
*/
static VALUE rb_git_tree_entrycount_recursive(int argc, VALUE* argv, VALUE self)
{
git_tree *tree;
Data_Get_Struct(self, git_tree, tree);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to move this statement below other variable declarations, as this generates a warning. (See the travis build logs).

int error;
struct rugged_treecount_cb_payload payload;
VALUE rb_limit;

rb_scan_args(argc, argv, "01", &rb_limit);

payload.limit = -1;
payload.count = 0;

if (!NIL_P(rb_limit)) {
Check_Type(rb_limit, T_FIXNUM);
payload.limit = FIX2INT(rb_limit);
}


error = git_tree_walk(tree, GIT_TREEWALK_PRE, &rugged__treecount_cb, (void *)&payload);

if(error && giterr_last()->klass == GITERR_CALLBACK) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Add a whitespace after if.

giterr_clear();
error = 0;
}

rugged_exception_check(error);

return INT2FIX(payload.count);
}

/*
* call-seq:
* tree[e] -> entry
Expand Down Expand Up @@ -813,6 +875,7 @@ void Init_rugged_tree(void)
*/
rb_cRuggedTree = rb_define_class_under(rb_mRugged, "Tree", rb_cRuggedObject);
rb_define_method(rb_cRuggedTree, "count", rb_git_tree_entrycount, 0);
rb_define_method(rb_cRuggedTree, "count_recursive", rb_git_tree_entrycount_recursive, -1);
rb_define_method(rb_cRuggedTree, "length", rb_git_tree_entrycount, 0);
rb_define_method(rb_cRuggedTree, "get_entry", rb_git_tree_get_entry, 1);
rb_define_method(rb_cRuggedTree, "get_entry_by_oid", rb_git_tree_get_entry_by_oid, 1);
Expand Down
6 changes: 6 additions & 0 deletions test/tree_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ def test_read_tree_data
assert_equal @oid, @tree.oid
assert_equal :tree, @tree.type
assert_equal 3, @tree.count
assert_equal 6, @tree.count_recursive
assert_equal 5, @tree.count_recursive(5)
assert_equal 6, @tree.count_recursive(10)
assert_raises(TypeError) do
@tree.count_recursive("NaN")
end
assert_equal "1385f264afb75a56a5bec74243be9b367ba4ca08", @tree[0][:oid]
assert_equal "fa49b077972391ad58037050f2a75f74e3671e92", @tree[1][:oid]
end
Expand Down