fix group/passwd api misuse if ZTS #13876
Conversation
This API is goofy and easy to misuse, the actual buffer size is unknown and the return of sysconf specifies an *initial buffer size* One has to iterate until function does not return ERANGE. Currently it works just because the initial buffer size is "enough" most of time.
| if (pwbuflen < 1) { | ||
| return FAILURE; | ||
| while ((retval = getpwnam_r(name, &pw, ZSTR_VAL(str), ZSTR_LEN(str), &retpwptr)) == ERANGE) { | ||
| str = zend_string_extend(str, (ZSTR_LEN(str) * 2) , 0); |
There was a problem hiding this comment.
overall makes sense, but I like the ZSTR_LEN(str) << 1 form better.
|
I¿m changing this to a draft.. further examination says at least php_fopen_primary_script has the same problem. |
Yes. and in any case this extra complexity may not be even worth it on ZTS. maybe, just maybe locking ourselves and calling the non thread safe version suffices. |
|
I don't know. I think the changes in your PR are fine and worth it. The added complexity is small. It improves reliability, and the impact on performance should be minimal as the initial buffer size is enough most of the time. Locking would comes with its lot of complexity as well. |
|
So, looking at all the the instances I found, The best solution that comes to mind is an internal API that sets one its parameters to a duplicated passwd/group structure..and further wrappers to "get" at least the data we used. |
|
We would need one wrapper per _r() function? It may not be worth it if we use each of these functions one or two times at most |
There was a problem hiding this comment.
It looks good to me. You don't need to necessarily fix all places in a single PR. So I would be fine to get this merged as is if you don't plan to implement the rest sometime soon.
If you plan to implement it for other places too, then I don't really mind if it's a shared API function or duplicated in each place. It's few lines so it's not such a big deal but API functions would be a bit cleaner I guess.
| grbuf = emalloc(grbuflen); | ||
| if (getgrnam_r(name, &gr, grbuf, grbuflen, &retgrptr) != 0 || retgrptr == NULL) { | ||
| efree(grbuf); | ||
| if(retval != 0 || retgrptr == NULL) { |
| pwbuf = emalloc(pwbuflen); | ||
| if (getpwnam_r(name, &pw, pwbuf, pwbuflen, &retpwptr) != 0 || retpwptr == NULL) { | ||
| efree(pwbuf); | ||
| if(retval != 0 || retpwptr == NULL) { |
|
There is actually a bug for this which we should put to NEWS when getting this merged (reminder for myself most likely): https://bugs.php.net/bug.php?id=68861 |
|
@crrodriguez @bukka let me know if I can help on this. I need this for #13925 :) |
|
@arnaud-lb I think you can just re-implement it to your PR or create a new one as this seems inactive and it's just a draft. We can't really wait forever if it's blocking your work. |
This API is goofy and easy to misuse, the actual buffer size is unknown and the return of sysconf specifies an initial buffer size
One has to iterate until function does not return ERANGE.
Currently it works just because the initial buffer size is "enough" most of time.