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

Fix CGo callback #2

Merged
merged 2 commits into from Apr 30, 2016
Merged

Fix CGo callback #2

merged 2 commits into from Apr 30, 2016

Conversation

aykevl
Copy link
Contributor

@aykevl aykevl commented Apr 16, 2016

It is not allowed to let C directly call Go functions, wrapper functions must be made. If it worked before, that was probably by accident.

Fixes #1

See: silvasur#1

It is not allowed to let C directly call Go functions, wrapper functions
must be made. For some reason, it appears to have worked before but it
doesn't work now.
@silvasur
Copy link
Owner

Sorry that I didn't reply sooner.

First: Thanks for the Pull request!

Well, it worked before (go1.1, i think), apparently cgo changed.

And it doesn't build for me, it seems the librsync API changed. Doesn't build with librsync1.0.0, which is the only version my distro offers. I'll look into this (might take some time).

@aykevl
Copy link
Contributor Author

aykevl commented Apr 20, 2016

Thank you for looking into it!

I expected it wouldn't work on version 1.0 as I had dug a bit into it as part of this fix, but my disto (Debian 8) only has version 0.9.7 or so.

The issue is that there was a security issue fixed in librsync in version 1.0.0. The 'strong' hash was only 8 bytes (64bit) long. The fact it was a very insecure hash (md4) didn't even matter a lot anymore at that size. As part of the fix, the constant for the default hash size has been removed and the hash changed to blake2b (which is not much slower but is cryptographically secure).

In a personal project I'm using 32 bytes (256bit) blake2b, which seems like a good default to me.

I am willing to fix librsync 1.0.0 compatibility if needed, but that'll take a few days. My other system has Debian testing with the new librsync, so I can test the changes.

@aykevl
Copy link
Contributor Author

aykevl commented Apr 30, 2016

Go 1.6 breaks cgo, again (they say for the last time).
I have written a workaround that fixes the issue, adhering to the new pointer rules in Go.
The repository is here:
https://github.com/aykevl/golibrsync/tree/fix-newer-versions
This is still not a fix for librsync 1.0.0 compatibility (turns out Debian stretch still uses the old version).

@aykevl
Copy link
Contributor Author

aykevl commented Apr 30, 2016

I have fixed librsync 1.0.0 compatibility. I have tested against both 0.9.7 (jessie, stretch) and 1.0.0 (experimental).
The API had to change a little bit or it would get cluttered, but updating is very simple and it's easier to change the API in the future without breaking compatibility.
The new code is in the same branch. Should I make a new pull request for that?

@silvasur silvasur merged commit 3a9208e into silvasur:master Apr 30, 2016
@silvasur
Copy link
Owner

Looks great! I like the API changes and it seems to work great with 1.0.0. Merging right now. Thank you very much!
I'd like to include you in the copyright file or a contibutors file, if you are okay with that?

@aykevl
Copy link
Contributor Author

aykevl commented Apr 30, 2016

:D

Yes, I'd like to be included in the contributors/license file.

@aykevl aykevl deleted the fix-cgo-callback branch April 30, 2016 22:10
@silvasur
Copy link
Owner

silvasur commented May 1, 2016

Okay, I added a CONTRIBUTORS file (commit bd612bb). If you don't like something about your entry there, feel free to send a pull request with the changes.

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

Successfully merging this pull request may close these issues.

None yet

2 participants