Fix ldap memory bugs for PHP 7.3 (fix: #79773) #5799
Conversation
ptomulik
force-pushed
the
fix-ldap-membugs-7.3
branch
2 times, most recently
from
3bb7632 to
3cd005c
Compare
ptomulik
force-pushed
the
fix-ldap-membugs-7.3
branch
from
3cd005c to
dda79dd
Compare
| @@ -397,7 +398,6 @@ static int _php_ldap_control_from_array(LDAP *ld, LDAPControl** ctrl, zval* arra | |||
| if (ber_flatten2(vrber, control_value, 0) == -1) { | |||
| rc = -1; | |||
| } | |||
| ber_free(vrber, 1); | |||
There was a problem hiding this comment.
Completely glossed over this line before ... this doesn't look right to me. Why does removing this not cause a memory leak? I don't see anything else that would free vrber.
There was a problem hiding this comment.
Somehow this caused a "Protocol error" which appeared to be a bad memory access (access to freed memory). I guess, this is specific to how the ber_flatten2() is implemented. If the third argument called alloc is zero, then it just assigns vrber->ber_buf to control_value->bv_val without allocating memory for control_value. The memory is released later with ber_memfree(control_value) at line 618. From what I've seen, this is recurring pattern in ldap.c - I couldn't find other pieces of code with ber_free() following ber_flatten2().
There was a problem hiding this comment.
Thanks for the explanation, that makes sense!
There was a problem hiding this comment.
Hm... just got back to this, and it looks like I could be wrong. The memory allocated for vrber->ber_buf is probably freed with ber_memfree(control_value), but there is still memory allocated for vrber itself. So removing completely this line was wrong. My bad. Perhaps we should replace it with ber_free(vrber, 0), or move the ber_free(vrber, 1) near the end of the function.
|
I've merged the use-after-free / memory-leak fixes into 7.3 as 23ef0a1. I'm going to apply the CI changes for 7.4 upwards. |
This PR enables
ext/ldap/testson travis and fixes some ldap memory bugs as described in #79773.