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 pointer to AEAD context #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silkeh
Copy link
Contributor

@silkeh silkeh commented Jul 16, 2017

The crypto_aead functions with precomputation take a pointer to a
crypto_aead_aes256gcm_state object as argument, not a char array.

The compiler is changed to clang because the code with the correct pointer does
not compile with GCC, see golang/go#7270.

@redragonx
Copy link
Contributor

Does it compile on Windows too?

@silkeh
Copy link
Contributor Author

silkeh commented Aug 13, 2017

I don't see why not, but I don't have any way to verify.

@redragonx
Copy link
Contributor

Hmm, ok.

Me neither, the batch file needs updating before I can accept this PR. Maybe it's a small change.

The crypto_aead functions with precomputation take a pointer to a
`crypto_aead_aes256gcm_state` object as argument, not a char array.

The compiler is changed to clang because the code with the correct pointer does
not compile with GCC, see golang/go#7270.
@silkeh
Copy link
Contributor Author

silkeh commented Aug 24, 2017

I have changed the batch file as well.

@theodelrieu
Copy link

Is there any news on this? It doesn't compile on macos with apple-clang 9.0 either, this patch fixes it

@theodelrieu
Copy link

It's weird that it breaks GCC, crypto_aead_aes256gcm_state is just a C typedef. This PR fixed clang but broke GCC.

Do you think there is a way to make both happy?

@jedisct1
Copy link
Contributor

jedisct1 commented Oct 4, 2017

That structure must be stored at a 16 bytes aligned location.

The same thing applies to crypto_onetimeauth_poly1305_state.

@silkeh
Copy link
Contributor Author

silkeh commented Oct 4, 2017

@theodelrieu GCC has an open issue for this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81001

@jedisct1
Copy link
Contributor

jedisct1 commented Oct 4, 2017

This seems to explain how to deal with alignment constraints: http://www.tapirgames.com/blog/golang-memory-alignment

@silkeh
Copy link
Contributor Author

silkeh commented Oct 4, 2017

@jedisct1 I will look into alignment constraints for this (and for #31), thanks!

@redragonx
Copy link
Contributor

Thank you to everyone that's helping with this issue.

@silkeh
Copy link
Contributor Author

silkeh commented Oct 7, 2017

I have updated #31 and created #32 for the alignment issue.

@redragonx
Copy link
Contributor

Does this work for CLANG and GCC now then?

@silkeh
Copy link
Contributor Author

silkeh commented Oct 23, 2017

@redragonx no, that is not possible until this GCC bug is solved.

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.

4 participants