proc_open refactoring (1) #5507

alexdowad · 2020-05-01T17:32:55Z

Code cleanup and reorganization in proc_open.c, in readiness to implement PTY support.

Code review will be easier if you look through commit by commit. However, if you want them squashed together before actually merging, that is fine.

alexdowad · 2020-05-01T21:00:11Z

Still fixing this up, sorry...

nikic · 2020-05-02T09:11:13Z

AppVeyor failure may be legit, seems like it's hanging?

cmb69 · 2020-05-02T10:57:18Z

It seems to me that the refactoring doesn't take into account that php_file_descriptor_t is a file descriptor (int) on POSIX systems, but may be an actual HANDLE on Windows; there are some conversions to a file descriptor, but the old code used CloseHandle() two times, while the refactored code no longer does.

nikic

Some quick notes, I'll have to check this more carefully.

nikic · 2020-05-02T13:35:42Z

ext/standard/proc_open.c

-	add_assoc_long(return_value, "termsig", termsig);
-	add_assoc_long(return_value, "stopsig", stopsig);
+	add_assoc_long(return_value, "termsig",  termsig);
+	add_assoc_long(return_value, "stopsig",  stopsig);


This alignment is not necessary.

Do you mean you don't want things to be aligned (if it means adding whitespace)? Of course it's not necessary; I just did it because personally I find it easier to work on code that way.

If you don't like it, please just mention.

For this project, unless there is a strong reason to do it, I'd avoid alignment. (This is a complication of tab indentation, which means that half the people end up trying to tab-align as well, and we get a mess in the end. Every time I edit aligned code, I have to check a few lines to figure out whether it is predominantly tab-aligned or space-aligned.)

ext/standard/proc_open.c

alexdowad · 2020-05-02T15:15:45Z

@nikic @cmb69 I am very impressed at the thoroughness of your review. This PR is still a WIP, but I will go through right now and respond to each of your comments.

alexdowad · 2020-05-02T15:18:32Z

It seems to me that the refactoring doesn't take into account that php_file_descriptor_t is a file descriptor (int) on POSIX systems, but may be an actual HANDLE on Windows; there are some conversions to a file descriptor, but the old code used CloseHandle() two times, while the refactored code no longer does.

You mean the bit where all open file handles are closed if spawning a process fails on Windows? The refactored code uses a macro which expands to CloseHandle on Windows. That macro was already there before, but wasn't fully used to reduce code duplication.

But if there is any other place which I missed, I would appreciate being reminded.

proc_open() is a huge function. Pulling out various pieces into helper functions will make it much easier to read and follow.

alexdowad · 2020-05-02T21:38:59Z

Still fixing things up... will push an update soon.

alexdowad · 2020-05-03T20:19:34Z

I think we're ready for more code review. Thanks very much for the great suggestions for improvement made thus far.

alexdowad · 2020-05-03T20:55:41Z

CI failure on Azure: In one of the jobs, apt failed to install libxml2 and libxslt, so all the test suite runs failed in that job.

alexdowad · 2020-05-04T08:16:28Z

Another branch is ready which enables PTY support (with a simplified implementation suggested by @nikic). It's based on this one, so maybe I should wait for this one to be reviewed before pushing it...

nikic

This looks good, a few notes.

ext/standard/proc_open.c

nikic · 2020-05-04T08:32:12Z

ext/standard/proc_open.c

-	add_assoc_long(return_value, "termsig", termsig);
-	add_assoc_long(return_value, "stopsig", stopsig);
+	add_assoc_long(return_value, "termsig",  termsig);
+	add_assoc_long(return_value, "stopsig",  stopsig);


For this project, unless there is a strong reason to do it, I'd avoid alignment. (This is a complication of tab indentation, which means that half the people end up trying to tab-align as well, and we get a mess in the end. Every time I edit aligned code, I have to check a few lines to figure out whether it is predominantly tab-aligned or space-aligned.)

ext/standard/proc_open.c

nikic · 2020-05-04T09:09:16Z

ext/standard/proc_open.c

+  /* zend_strings which may be allocated while processing the descriptorspec
+   * we need to make sure that each one allocated is released once and only once */
+	zend_string *ztype = NULL, *zmode = NULL, *zfile = NULL;
+#define cleanup_zend_string(ptr) do { if (ptr != NULL) { zend_string_release(ptr); ptr = NULL; } } while(0)


This is zend_tmp_string_release().

OK. Though the macros are still needed.

The point of these macros is that the function is loaded with goto exit_fail;s and it would be cumbersome to ensure that the correct zend_strings are released at each such point. So when releasing them, we set the pointer to NULL so there's no danger of freeing them twice.

I see. I don't really like this, but let's leave it for now.

In the interest of reducing non-local state, I've moved the code that uses these strings into a separate function in 2dc4481.

ext/standard/proc_open.c

nikic · 2020-05-04T09:21:02Z

ext/standard/proc_open.c

 				if (!ztarget) {
 					zend_value_error("Missing redirection target");
 					goto exit_fail;
 				}
 				if (Z_TYPE_P(ztarget) != IS_LONG) {
-					zend_value_error("Redirection target must be an integer");
+					zend_value_error("Redirection target must be an integer; got %s", zend_get_type_by_const(Z_TYPE_P(ztarget)));


Suggested change

zend_value_error("Redirection target must be an integer; got %s", zend_get_type_by_const(Z_TYPE_P(ztarget)));

zend_value_error("Redirection target must be of type int, %s given", zend_get_type_by_const(Z_TYPE_P(ztarget)));

is the usual phrasing.

I think this part is not done yet.

I think I was still running make test and hadn't pushed yet when you made this comment.

ext/standard/proc_open.c

Every time proc_open() is called on Windows, an identical SECURITY_ATTRIBUTES struct is initialized. It is never modified, just used to tell CreateProcessW() that subprocesses are allowed to inherit file handles from the parent. Just allocate one of them statically and be done with it.

This field has 3 possible values: DESC_FILE, DESC_PIPE, and DESC_REDIRECT, but the code only cares about whether the value is DESC_PIPE or not. Therefore, make the code simpler and clearer by converting the field to a boolean called 'is_pipe'.

Setting these pointers to NULL after freeing them was necessary because if an error occurred later on, they could be double-freed after 'goto exit_fail'. It is simpler to free them after all possible sources of error are past.

alexdowad · 2020-05-04T12:02:42Z

I've noticed there is also a TODO comment left behind:

/* TODO Deprecate in favor of array command? */

Do you think that would be a good idea? (Deprecating the bypass_shell option on Windows in favor of the newer array-as-command style invocation of proc_open.)

…tegral

This fix courtesy of Nikita Popov.

cmb69 · 2020-05-04T13:17:57Z

Do you think that would be a good idea? (Deprecating the bypass_shell option on Windows in favor of the newer array-as-command style invocation of proc_open.)

May be, but I think that should be discussed on the internals@ mailing list first.

alexdowad · 2020-05-04T14:49:11Z

I must say I love the thorough code reviews on this project.

alexdowad force-pushed the proc-open-refactoring-1 branch 4 times, most recently from 6c91aec to b0f9b8f Compare May 1, 2020 20:15

alexdowad force-pushed the proc-open-refactoring-1 branch from b0f9b8f to 905e4a9 Compare May 1, 2020 21:26

nikic reviewed May 2, 2020

View reviewed changes

cmb69 added the Feature label May 2, 2020

alexdowad added 3 commits May 2, 2020 21:01

Nothing in proc_open.c needs stat.h

3459039

Introduce set_proc_descriptor_to_blackhole helper in proc_open.c

8a1981d

proc_open() is a huge function. Pulling out various pieces into helper functions will make it much easier to read and follow.

Remove unneeded constant DESC_PARENT_MODE_WRITE from proc_open.c

838fbc1

alexdowad force-pushed the proc-open-refactoring-1 branch 4 times, most recently from ab74c7b to 0c74bd2 Compare May 3, 2020 19:41

nikic reviewed May 4, 2020

View reviewed changes

alexdowad added 6 commits May 4, 2020 13:09

Code tweaks in proc_open.c

0a3e051

Introduce efree_argv helper in proc_open.c

3bec2e1

Introduce get_option helper in proc_open.c

1a534a2

Introduce alloc_descriptor_array helper in proc_open.c

292efd4

Introduce set_proc_descriptor_to_pipe helper in proc_open.c

a058757

alexdowad added 13 commits May 4, 2020 13:15

Introduce init_startup_info helper in proc_open.c

f971744

Introduce close_all_descriptors helper in proc_open.c

c8ca391

Introduce set_proc_descriptor_to_file helper in proc_open.c

a0a290b

Introduce convert_command_to_use_shell helper in proc_open.c

7484ec4

Introduce init_process_info helper in proc_open.c

fd524a6

Introduce close_parent_ends_of_pipes_in_child helper in proc_open.c

14783fe

Show error in child process if exec fails (in proc_open)

a62699a

Simplify code for finding descriptor to redirect to

644a1bc

Free memory just before returning from proc_open

6a981a0

Setting these pointers to NULL after freeing them was necessary because if an error occurred later on, they could be double-freed after 'goto exit_fail'. It is simpler to free them after all possible sources of error are past.

Introduce dup_proc_descriptor helper in proc_open.c

fa49945

Simplify code for handling streams in proc_open

8dbf742

Introduce get_string_parameter helper in proc_open.c

1b4ac55

alexdowad added 6 commits May 4, 2020 14:10

Introduce get_command_from_array helper in proc_open.c

a15dc8d

Use zend_string for strings in proc_open

3c99c0b

Remove long-disabled #defines from proc_open.c

38b2b0f

Introduce redirect_proc_descriptor helper in proc_open.c

0f653c6

Improve error message when FD redirect target for proc_open is non-in…

6e1e5c6

…tegral

Dereference references used in 'other_options' parameter to proc_open

c1922e8

This fix courtesy of Nikita Popov.

alexdowad force-pushed the proc-open-refactoring-1 branch from 5cd6a91 to c1922e8 Compare May 4, 2020 12:24

php-pulls closed this in 51b0494 May 4, 2020

	zend_value_error("Redirection target must be an integer; got %s", zend_get_type_by_const(Z_TYPE_P(ztarget)));
	zend_value_error("Redirection target must be of type int, %s given", zend_get_type_by_const(Z_TYPE_P(ztarget)));

proc_open refactoring (1) #5507

proc_open refactoring (1) #5507

Uh oh!

Conversation

alexdowad commented May 1, 2020

Uh oh!

alexdowad commented May 1, 2020

Uh oh!

nikic commented May 2, 2020

Uh oh!

cmb69 commented May 2, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexdowad commented May 2, 2020

Uh oh!

alexdowad commented May 2, 2020

Uh oh!

alexdowad commented May 2, 2020

Uh oh!

alexdowad commented May 3, 2020

Uh oh!

alexdowad commented May 3, 2020

Uh oh!

alexdowad commented May 4, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexdowad commented May 4, 2020

Uh oh!

cmb69 commented May 4, 2020

Uh oh!

alexdowad commented May 4, 2020

Uh oh!

Uh oh!

nikic May 4, 2020 •

edited

Loading