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

Fixes for ZPP string arguments (and stuff) #810

Closed
wants to merge 10 commits into from

Conversation

datibbaw
Copy link
Contributor

@datibbaw datibbaw commented Sep 6, 2014

I stepped through all usages of zend_parse_parameters() in all core extensions except for ext/standard to make sure the correct type is used for the string length.

At the same time I've looked at overzealous replacements of int with size_t; some values work with negative numbers and as such should remain as int.

Finally, some minor inconsistencies were addressed.

This is a PR for two reasons:

  1. Gather more feedback before submission
  2. Let it run through Travis CI

@@ -1426,7 +1427,8 @@ static void php_sybase_query (INTERNAL_FUNCTION_PARAMETERS, int buffered)
zval *sybase_link_index = NULL;
zend_bool store = 1;
char *query;
size_t len, id, deadlock_count;
size_t len, deadlock_count;
int id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deadlock_count doesn't sound very size_tish. Also used with %d below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! 😺

@jpauli
Copy link
Member

jpauli commented May 13, 2015

Is this PR still relevant now ?
I think most of work has been implemented so far

@nikic
Copy link
Member

nikic commented Mar 3, 2016

There are still relevant parts here. One change in interbase.c and several in mbstring relating to the mbfl_string structure.

I wonder how mbfl_string interacts with our ABI, can we change it in a patch release?

@nikic
Copy link
Member

nikic commented Mar 3, 2016

Interbase fix: a7028d9

We do export the mbfl_string structure in headers, so can't change it for 7.0. So we should port mbfl to use size_t in master and use the solution in this PR for 7.0.

@weltling
Copy link
Contributor

weltling commented Mar 7, 2016

@nikic you are right! mbfl_string is not compatible with zend_string on 64-bit, thanks for being aware. I've pushed this 89a4342 to fix the most of issue. There is probably some more to go, but almost nothing to do with incompatible pointers for string lengths.

Thanks.

@nikic
Copy link
Member

nikic commented Mar 7, 2016

@weltling Thanks! With that commit all changes from this PR should be covered, so closing here.

@nikic nikic closed this Mar 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants