search

Noblis / INVSC-janus

IARPA Janus Program API
Other
9 stars 16 forks source link

Fixing a write-after-end-of-array bug in janus_evaluate_search #22

Closed stephenrawls closed 9 years ago

stephenrawls commented 9 years ago

Hi Janus Dev Team,

In testing our API I ran into a segfault in the case where the number of actual returned search results was less than the number of requested returned search results.

I ended up tracing the problem to a bug in janus_io.cpp. Essentially the similarity_matrix array was being indexed as if the code were accessing it from the start of the array, but really the similarity_matrix pointer was getting incremented during the loop so the indexing should have been relative to the current row.

This pull request fixes that bug. It also adds an extra check to see if the janus_search function ended up returning more search results than requested. I just figured if I was making changes I might as well add this check in as well.

Thanks, Stephen Rawls (ISI)

stephenrawls commented 9 years ago

P.S. Not sure how you want to handle the phase 1 branch, but this bug is also present in the phase 1 branch.

bhklein commented 9 years ago

Thanks Stephen this looks good to me. I'll update the phase1 branch once you merge this.

Ben

stephenrawls commented 9 years ago

Hi Ben. I removed the now obsolete variable as suggested. Do you mean I should merge into the master branch? I'm still new to github and unsure of the correct protocol--I created a feature branch for my pull request because I thought maybe that's what I was supposed to do. Should I have instead just given a pull request for the master branch?

bhklein commented 9 years ago

Hi Stephen, yes I did mean to merge into the biometrics:master, which I've just realized that you cannot since you don't have write access. Sorry about that!

stephenrawls commented 9 years ago

Hi Ben,

Sorry for the confusion, I thought you meant I was suppose to merge into master, which I was unsure of how to do (e.g. merge into master in my fork? how would that effect this pull request?).

Looks like you merged into master, so that's good.

Quick question: should we send a courtesy e-mail to dev@libjanus.org to point folks to this change in case other teams encounter the bug?

Thanks! Stephen

bhklein commented 9 years ago

Yes if you could send a quick email to the mailing list that would be appreciated, thanks!

stephenrawls commented 9 years ago

Ok will do. Just to confirm: you are going to change phase1 branch as well? Or do I need to issue another pull request?

bhklein commented 9 years ago

No I will change the phase1 branch, thanks.