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.

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

Hash mismatch with reference implementation #5

Closed
zhenjl opened this issue Jul 26, 2017 · 11 comments · Fixed by #6
Closed

Hash mismatch with reference implementation #5

zhenjl opened this issue Jul 26, 2017 · 11 comments · Fixed by #6
Assignees
Labels

Comments

@zhenjl
Copy link

zhenjl commented Jul 26, 2017

Hi, thanks for the pure go library for ssdeep. I was curious if you have compared results with github.com/dutchcoders/gossdeep. I just did a quick test and it seems like the results are slightly diff:

"12288:+AxbaNI5pVxkQw3iNjQzTgLNh7EMusK8NiftLV1rq:+HITCchFHtNiftvq" (expected)
"12288:+AxbaNI5pVxkQw3iNjQzTgLNh7EMusK8NiftLV1rqs:+HITCchFHtNiftvqs" (actual)

First one is from @dutchcoders, second is from yours.

@glaslos glaslos added the bug label Jul 27, 2017
@glaslos
Copy link
Owner

glaslos commented Jul 27, 2017

Hi, thanks for your interest. You have probably noticed that gossdeep is just a wrapper around the official implementation. I still have some issues in mine (as you have rightful noticed) with handling some of the corner cases.

@glaslos glaslos self-assigned this Jul 27, 2017
@davidt99
Copy link
Contributor

Any progress on this issue? Do you have an idea where the problem is?

@glaslos
Copy link
Owner

glaslos commented Oct 17, 2017

Hi @davidt99 unfortunately I did not had time to look into this yet. How familiar are you with Go? I could give you some pointers that get you started investigating the problem.
To be clear, the dutchcoders implementation is a wrapper of the original library.

@davidt99
Copy link
Contributor

I just started writing in Go, but I feel comfortable enough with the language to try and fix the problem, so if it's not too much trouble, post the pointers and I will try to fix it.
@dutchcoders implementation uses "C" package (I guess there is no other option), and that has a performance penalty for every call - that why I'm interested in using your implementation.

@glaslos
Copy link
Owner

glaslos commented Oct 17, 2017

Yes, there are a bunch of downsides to using cgo: https://dave.cheney.net/2016/01/18/cgo-is-not-go
In https://github.com/glaslos/ssdeep/blob/master/ssdeep.go#L136 we call processByte which appends one more character to the hash. You basically have to find out why the original implementation stops one character earlier.

@glaslos glaslos changed the title Diff results than github.com/dutchcoders/gossdeep Hash mismatch with reference implementation Nov 29, 2017
@glaslos
Copy link
Owner

glaslos commented Nov 29, 2017

You might have noticed when comparing the results that our implementation is appending some extra characters to the hash. Might be a good starting point to find the root cause.

@glaslos
Copy link
Owner

glaslos commented Nov 30, 2017

This seems to be more significant with smaller files.

@davidt99
Copy link
Contributor

davidt99 commented Dec 5, 2017

I think I fixed it, but I need to do more testing. Do you have some sort of control group that I can use?

@zhenjl
Copy link
Author

zhenjl commented Dec 5, 2017

I can help with that since I filed the original bug :)

@davidt99
Copy link
Contributor

davidt99 commented Dec 5, 2017

I think I was able to reproduce the mismatch you described, but while I was trying to solve it, I found another mismatch (in a different location).
I want to be more sure that I fixed all the bugs, that's why I want more than just a few files to test with.
If you have a test group, please post it. The sample you first use is also a good start.

@glaslos glaslos closed this as completed in #6 Dec 6, 2017
glaslos pushed a commit that referenced this issue Dec 6, 2017
@glaslos
Copy link
Owner

glaslos commented Dec 6, 2017

@zhenjl please confirm that cf3f9ba fixed your issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants