Issue 43177 #98

Closed
wants to merge 6 commits into
from

Projects

None yet

7 participants

@jabouillei

I changed main.c and Zend.c to address bug #43177. The php.net description of eval() states:
"If there is a parse error in the evaluated code, eval() returns FALSE and execution of the following code continues normally."
That statement is almost true. The exit status and http response code were made 255 and 500. Even thought the script proceeds normally and the webpage may be perfectly constructed, the results may be discarded (e.g. by wget or an ajax framework) because of the 255/500. The changes to main.c and Zend.c in this request prevent an eval() error from affecting the exit status and http response code.

This is my first attempt to submit a bug fix to php. I believe I have followed the instructions at
https://wiki.php.net/vcs/gitworkflow
but I see that a half dozen files are listed as modified. I guess that is a merge issue, but I don't see how I am supposed to address it. I'm hoping it will be easy for whoever handles this pull request to only accept the relevant changes in main.c and zend.c.

Because the change regards the exit status and response code, testing does not appear to be compatible with the php test framework. I have not added anything to the tests/ directory.

Todd Ruth If an eval() has a parse error, the overall exit status and return
code should not be affected.  (bug 43177)  Without this fix,
a webpage using eval() may return code 500.  That might display
fine and the 500 go unnoticed, but using AJAX or wget, the 500
will cause problems.
I looked at existing tests to create a new test for this fix,
but I don't see a way to test the exit status or http response
code.  The test cases seem to rely on the textual output of
the test case.
a65037e
@nikic
Contributor
nikic commented Jun 2, 2012

Somethinig is wrong with your diff, it contains changes that weren't actually made by you. Could you do a clean rebase to master (or whatever your target branch is)?

@jabouillei

The instructions (https://wiki.php.net/vcs/gitworkflow) said to
"git pull --rebase upstream/PHP-5.4"
When I try that, it tells me that upstream/master is not a git repository
In parentheses it says "(or use upstream/master)"
That gives a similar error message.
I took a guess and tried php/php-src, but that didn't work either.
When I tried just plain "git pull --rebase" it told me the current
branch is up to date. Do you know what I'm supposed to do?
I followed all the instructions on that web page carefully, but
I guess it assumes you have some experience with git and
apparently some of the instructions weren't meant to be taken
literally. As a twice/year user of git, it isn't at all clear which
instructions should be taken literally and which involve replacing
parts of commands.

If there's a way to be selective in what you take, my changes were
all in a single commit affecting main.c and zend.c.

  • Todd

----- Original Message ----
From: nikic
reply@reply.github.com

To: jabouillei jabouillei@yahoo.com
Sent: Fri, June 1, 2012 7:00:44 PM
Subject: Re: [php-src] Issue 43177 (#98)

Somethinig is wrong with your diff, it contains changes that weren't actually
made by you. Could you do a clean rebase to master (or whatever your target
branch is)?


Reply to this email directly or view it on GitHub:
#98 (comment)

@jabouillei

This is my second attempt to submit this change. For my first attempt, I followed the instructions at https://wiki.php.net/vcs/gitworkflow carefully and received an error when attempting "git pull --rebase upstream/PHP-5.4". I guess that's why there were other people's commits showing up in my pull request. Since then, I've executed the following commands. Some portion of these command seemed to fix things and others may have done nothing or messed something up. If anyone reads this that has the knowledge and permission to update https://wiki.php.net/vcs/gitworkflow, please make the appropriate updates so other people don't experience the problems I hit. Here are the additional commands that seemed to help:

git fetch upstream
git merge upstream/master
git push origin issue-43177
git checkout master
git merge upstream/master
git push origin master

@jabouillei

I think I've fixed the pull request. The instructions for external contributors
didn't say
to do the following, but I executed the following commands and I think some
portion
of these commands helped:

git fetch upstream
git merge upstream/master
git push origin issue-43177
git checkout master
git merge upstream/master
git push origin master

After that I updated my pull request and now it only shows my changes.

If some portion of those commands are needed for external contributors and
you know who owns https://wiki.php.net/vcs/gitworkflow please update that
page so future external contributors don't hit the same problems. The
"git pull --rebase upstream/PHP-5.4" command shown on that page never
did work for me.

Thanks!

  • Todd

----- Original Message ----
From: nikic
reply@reply.github.com

To: jabouillei jabouillei@yahoo.com
Sent: Fri, June 1, 2012 7:00:44 PM
Subject: Re: [php-src] Issue 43177 (#98)

Somethinig is wrong with your diff, it contains changes that weren't actually
made by you. Could you do a clean rebase to master (or whatever your target
branch is)?


Reply to this email directly or view it on GitHub:
#98 (comment)

@smalyshev smalyshev and 2 others commented on an outdated diff Jun 8, 2012
@@ -1218,7 +1218,10 @@ ZEND_API void zend_error(int type, const char *format, ...) /* {{{ */
va_end(args);
if (type == E_PARSE) {
- EG(exit_status) = 255;
+ /* eval() errors do not affect exit_status */
+ if (EG(current_execute_data)->opline->extended_value != ZEND_EVAL) {
@smalyshev
smalyshev Jun 8, 2012 Contributor

Not sure this is safe - what guarantees that EG(current_execute_data)->opline is valid for every call to zend_error?

For example, this segfaults: php -r '0+'

@nikic
nikic Jun 8, 2012 Contributor

Also I'm not sure whether just checking the extended value is safe. ZEND_EVAL is 1 and I imagine that this extended value is also used for other opcodes in other circumstances. So you should probably additionally check that the opcode is ZEND_INCLUDE_OR_EVAL.

@jabouillei
jabouillei Jun 9, 2012

Glad someone who knows how internals works looked at this! I've updated the code to check for the existence of everything being used before using it. In main.c, I introduced a local boolean. I guessed the type should be "zend_bool", but that was just a guess.

php -r '0+' no longer segfaults. Bad evals are harmless. Other parse errors are fatal (but don't segfault).

Todd Ruth added some commits Jun 8, 2012
@smalyshev smalyshev and 1 other commented on an outdated diff Aug 20, 2012
@@ -1112,6 +1112,11 @@ static void php_error_cb(int type, const char *error_filename, const uint error_
}
/* Bail out if we can't recover */
+ /* eval() errors do not affect exit_status or response code */
+ zend_bool during_eval = (EG(current_execute_data) &&
@smalyshev
smalyshev Aug 20, 2012 Contributor

I'm afraid you can't do this in plain C - you need to define vars in the beginning of the function.

@jabouillei
jabouillei Aug 21, 2012

I've fixed this. I saw the other mid-function definitions and thought it was
OK, but now I understand that the definition must be at the beginning of
a block. Sorry. Rather than add confusion by putting the definition and
perhaps
assignment at the top of the function, I've introduced a block where the
variable is needed.

@travisbot

This pull request fails (merged 511cb36 into 460d258).

@jabouillei

If I should do something about this, please let me know. Apparently, this was
an automated test. I'm not sure what triggered it. I was trying a variety of
"git" commands in hopes of bringing my branch up to date. Perhaps one of those
commands caused this.

@laruence
Member

you can ignore the failed report by travisbot... it always reports failed

@lstrojny
Contributor

@jabouillei Could you add test cases with the built in webserver to make sure your patch behaves like it should?

@smalyshev
Contributor

Some test cases are definitely needed.

@smalyshev
Contributor

With this patch, this code:

eval("foo();");

will return 200 too, even though it results in a fatal error. This is not right, so we need some better solution here.

@php-pulls

Comment on behalf of stas at php.net:

merged

@php-pulls php-pulls closed this Jan 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment