Created attachment 213917 [details] [review]
girepository: avoid crash when querying nonexistent info

It appears that cmph library can return (n+1) when querying item not
present in its original n-item-sized set.  Adjust code so that it
detects this condition and do not chase stray pointers resulting from
this bogus hash result.

Review of attachment 213917 [details] [review]:

::: girepository/gitypelib.c
@@ +190,2 @@
       index = _gi_typelib_hash_search (hash, name);
+      if (index < n_entries)

This seems wrong to me; note that inside _gi_typelib_hash_search, we use the integer returned by BDZ as an index into another lookup table.  See the comment in girepository/gthash.c.

If the bug indeed lies inside BDZ, then the right fix is probably to detect this inside _gi_typelib_hash_search().  That function is supposed to always return a valid value though...

This needs more investigation.

(In reply to comment #2)
> Review of attachment 213917 [details] [review]:
> 
> ::: girepository/gitypelib.c
> @@ +190,2 @@
>        index = _gi_typelib_hash_search (hash, name);
> +      if (index < n_entries)
> 
> This seems wrong to me; note that inside _gi_typelib_hash_search, we use the
> integer returned by BDZ as an index into another lookup table.  See the comment
> in girepository/gthash.c.

You are right, I knew that but somehow forgot it when writing a patch.  I'll try to come with something better.

> If the bug indeed lies inside BDZ, then the right fix is probably to detect
> this inside _gi_typelib_hash_search().  That function is supposed to always
> return a valid value though...

Given that for the testcase appended in the patch BDZ really generates out-of-range index, I guess we have to somehow handle it.  I suggest to detect this index overrun inside _gi_typelib_hash_search and return 0-th entry of the lookup table (as if BDZ returned 0).  Then later g_typelib_get_dir_entry_by_name() detects that names differ (using strcmp) and DTRT.  Preparing patch right now...

> This needs more investigation.

I guess that the main question is whether BDZ behaviour is correct in this case; but I'm absolutely uneducated in the area of hash/crypto calculations, so someone else would have to check the BDZ algo/impl for the correctness in this case.

Created attachment 213932 [details] [review]
girepository: avoid crash when querying nonexistent info

It appears that cmph library can return (n+1) when querying item not
present in its original n-item-sized set.  Adjust code so that it
detects this condition and do not chase stray pointers resulting from
this bogus(?) hash result.

Review of attachment 213932 [details] [review]:

Looks good, thank you!

Attachment 213932 [details] pushed as d055d35 - girepository: avoid crash when querying nonexistent info

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]