Fix compiler warnings in proc_open.c #5629
Conversation
We are getting a lot of compiler warnings when building on Appveyor CI (Windows). Fix some of them.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agree. But if you want it done... just say so. I am willing.
These are causing compiler warnings when building in Appveyor CI. (Also slipped in a couple style and grammar tweaks.)