Skip to content

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

Merged
merged 8 commits into from
Dec 9, 2013

Conversation

bukka
Copy link
Member

@bukka bukka commented Dec 1, 2013

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

@bukka
Copy link
Member Author

bukka commented Dec 1, 2013

The branches are merged and new fixes have been added. Also some overflow checks are in place...

@weltling
Copy link
Contributor

weltling commented Dec 2, 2013

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?

@weltling
Copy link
Contributor

weltling commented Dec 2, 2013

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 :)

@bukka
Copy link
Member Author

bukka commented Dec 2, 2013

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 ;)

@bukka
Copy link
Member Author

bukka commented Dec 8, 2013

@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 ZEND_UINT_FMT but not sure if it's a good idea for zend_str_size_int. I saw that you added %pd modifier for php_int_t. What do you want to use for zend_str_size_int? I was thinking about using %zd as it's for size_t but maybe new modifier would be better?

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).

@php-pulls php-pulls merged commit d3acd87 into php:str_size_and_int64 Dec 9, 2013
@weltling
Copy link
Contributor

weltling commented Dec 9, 2013

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

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.

3 participants