OpenSSL checks for str_size_and_int64 #536

Merged
merged 8 commits into from Dec 9, 2013

3 participants

@bukka

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

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

@weltling

This is too much, 'n' is passed directly to X509_digest. On win64 the later pointer cast won't work as expected too.

yeah this should be changed to unsigned int. I am gonna do that

@weltling

This cast is useless, len is positive and will never overflow size_t.

I think that it's quite useful to have these casting to see where the conversion are. There also is another reason for it - it's suppressing warnings when you compile with -Wconversion on gcc

@weltling

This cast is useless, bio_mem_len is positive and won't overflow size_t.

@weltling

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

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

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

@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

1 check failed

Details default The Travis CI build could not complete due to an error
@weltling

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