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

zmq_z85_decode documentation is ambiguous about error behaviour #2321

Closed
sigiesec opened this issue Jan 24, 2017 · 5 comments
Closed

zmq_z85_decode documentation is ambiguous about error behaviour #2321

sigiesec opened this issue Jan 24, 2017 · 5 comments

Comments

@sigiesec
Copy link
Member

The documentation of zmq_z85_decode is ambiguous about its behaviour in case of invalid input.
It states "The encoding shall follow the ZMQ RFC 32 specification." and "The zmq_z85_decode() function shall return dest if successful, else it shall return NULL."

As implemented, it returns NULL if the length of the input is invalid, but not if it contains invalid characters. This should at least be clarified in the documentation.

However, I think that the method should be changed such that it either

  • assumes that the input is completely valid (and have undefined behaviour otherwise), or
  • validates its input more thoroughly.

I am not sure which of these alternatives better fits in with the overall API design.

@bluca
Copy link
Member

bluca commented Jan 24, 2017

Given the doc says it should follow RFC32 and it's not really usable if the input is invalid anyway, I think in this case it would be acceptable to change the function to do more validation. Could you please send a PR to fix it?

@sigiesec
Copy link
Member Author

@bluca I can provide a PR... I already made some provisional change (2290552). However, I also wanted to add tests, and have the problem that the tests are not included in the VS2015 solution.

@bluca
Copy link
Member

bluca commented Jan 24, 2017

How come they are not? Can it be fixed?

Also the CI will run them, and you can enable it on your fork, just go to:

https://travis-ci.org/
https://www.appveyor.com/

And follow instructions to sign up with your Github account and enable your libzmq fork

@sigiesec
Copy link
Member Author

The VS2015 only contains the libzmq project, and dangling references to the perf programs, which seem not to have been copied when the VS2015 solution was forked from the VS2013 solution. I managed to add, build and run my test locally, but I am not sure if this is correct in general. Also, adding the tests is quite cumbersome, as a project file and properties file needs to be added for each test file.

@evoskuil
Copy link
Contributor

evoskuil commented Jan 25, 2017

The VS projects should all be identical with a few minor exceptions (except for 2008, which IMO we should drop support for). I have code that generates them but don't run it very often. The 2015 solution was added independently of that process. I've been planning to get back and clean them up again, but have been pretty busy for a while.

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

No branches or pull requests

3 participants