-
Notifications
You must be signed in to change notification settings - Fork 7.8k
OpenSSL checks for str_size_and_int64 #536
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
OpenSSL checks for str_size_and_int64 #536
Conversation
The branches are merged and new fixes have been added. Also some overflow checks are in place... |
Jakub, it looks almost good, the only one bad thing came out after the tests was that with X509_digest(). Besides that, I would really remove unnecessary type casts. Also, some casts look suspicious but not unnecessary bad, but anyway ... i mean casts int to size_t might end bad if the int variable is negative, whereby an explcit cast will suppress any warnings ... could you please double check such places? |
When i compile on windows with static analyzer, there are tons of warnings of this art, i mean even now. Right, after you've incorporated the range checks, those are almost false positives. Let's fix the bad thing first, i'll test again then :) |
Thanks for the comments. I'll fix the X509_digest thing. ;) The casts were mainly for me to see where the conversions are. After I will resolve all issues on the TODO list (mainly overflow checks), I will go through it once again and then remove the unnecessary type casts. I will let you know then ;) |
@weltling Could you test it on win pls? :) I have added overflow checks and removed the unnecessary type casts. I also double checked with OpenSSL (API types changes) so that should be ok as well. I will probably need to do one more round to checks if I didn't forget anything. Maybe some extra tests for 64bit would be good as well? The fmt messages will need to be probably changed. Currently I'm using In addition there could be better solution for ctx based methods but think that it would better to merge them to the master first as it should not have any negative performance issues and it will be easier to keep str_size_and_int64 synced with master. I came across few other things (including few cases of incorrectly used OpenSSL API) that could be improved. I plan to send more patches in the future (as a separate PR for master). |
Jakub, thanks for the patch, merged! If you have time to add some explicit tests for 64 bit, that makes of course sense. For now everything passes in my tests. With the zend_str_size format - yes, %zd is already there. Whereby i think it's also safe to use %pu (the p modifier was implemented also for o and X). Cheers Anatol |
This is for str_size_and_int64 branch (Anatol).
It's in progres so please don't merge it yet!
I have created this PR just to make it easier to discuss the changes and possible issues.
TODO list (progress of this patch) is available at https://gist.github.com/bukka/7617548