Skip to content

Fix compiler warnings in proc_open.c #5629

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix compiler warnings in proc_open.c #5629

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b264a01
Avoid compiler warnings in proc_open.c
alexdowad May 26, 2020
7554d37
Tweak style and grammar in proc_open.c
alexdowad May 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions ext/standard/proc_open.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,18 @@
/* This symbol is defined in ext/standard/config.m4.
* Essentially, it is set if you HAVE_FORK || PHP_WIN32
* Other platforms may modify that configure check and add suitable #ifdefs
* around the alternate code.
* */
* around the alternate code. */
#ifdef PHP_CAN_SUPPORT_PROC_OPEN

#if HAVE_OPENPTY
# if HAVE_PTY_H
# include <pty.h>
# else
# if defined(__FreeBSD__)
# elif defined(__FreeBSD__)
/* FreeBSD defines `openpty` in <libutil.h> */
# include <libutil.h>
# else
/* Mac OS X and some BSD defines `openpty` in <util.h> */
# include <util.h>
# endif
# include <libutil.h>
# else
/* Mac OS X (and some BSDs) define `openpty` in <util.h> */
# include <util.h>
# endif
#endif

Expand All @@ -74,7 +71,7 @@ static php_process_env _php_array_to_envp(zval *environment)
char **ep;
#endif
char *p;
size_t cnt, sizeenv = 0;
size_t sizeenv = 0;
HashTable *env_hash; /* temporary PHP array used as helper */

memset(&env, 0, sizeof(env));
Expand All @@ -83,7 +80,7 @@ static php_process_env _php_array_to_envp(zval *environment)
return env;
}

cnt = zend_hash_num_elements(Z_ARRVAL_P(environment));
uint32_t cnt = zend_hash_num_elements(Z_ARRVAL_P(environment));

if (cnt < 1) {
#ifndef PHP_WIN32
Expand Down Expand Up @@ -794,7 +791,7 @@ static int set_proc_descriptor_from_array(zval *descitem, descriptorspec_item *d
}

retval = redirect_proc_descriptor(
&descriptors[ndesc], Z_LVAL_P(ztarget), descriptors, ndesc, nindex);
&descriptors[ndesc], (int)Z_LVAL_P(ztarget), descriptors, ndesc, nindex);
Copy link
Member

Choose a reason for hiding this comment

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

If we want to be pedantic, we can add a if (ZEND_LONG_EXCEEDS_INT(Z_LVAL_P(ztarget)) { error } check beforehand, to avoid silently truncating the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's worth it. But if you want...

Will people will ever call proc_open with file descriptor numbers so big they don't fit in an int? Do you know any Unix kernel which allows processes to use that many file descriptors?

At other places in proc_open we also truncate the FD numbers to int. Rather than fixing it here, it would make more sense to check right at the beginning where the arguments are checked. (But as said, I don't think it's worth it.)

Copy link
Member

Choose a reason for hiding this comment

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

In this case the issue is the other way around: Not that you might have this many file descriptors (which, as you say, is impossible), but that you could write ['redirect', 0x100000001] and have it silently behave the same as ['redirect', 1].

But as you say, probably not worth the bother. It's not like it would actually break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. But if you want it done... just say so. I am willing.

} else if (zend_string_equals_literal(ztype, "null")) {
/* Set descriptor to blackhole (discard all data written) */
retval = set_proc_descriptor_to_blackhole(&descriptors[ndesc]);
Expand Down Expand Up @@ -829,7 +826,7 @@ static int set_proc_descriptor_from_resource(zval *resource, descriptorspec_item
}

#ifdef PHP_WIN32
php_file_descriptor_t fd_t = (HANDLE)_get_osfhandle(fd);
php_file_descriptor_t fd_t = (php_file_descriptor_t)_get_osfhandle(fd);
#else
php_file_descriptor_t fd_t = fd;
#endif
Expand Down Expand Up @@ -991,8 +988,8 @@ PHP_FUNCTION(proc_open)
goto exit_fail;
}
} else if (Z_TYPE_P(descitem) == IS_ARRAY) {
if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, nindex, &pty_master_fd,
&pty_slave_fd) == FAILURE) {
if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, (int)nindex,
&pty_master_fd, &pty_slave_fd) == FAILURE) {
goto exit_fail;
}
} else {
Expand Down