Keep EG(errors) buffer consistent on erealloc failure

[edorian]

Update the error count only after the buffer has been resized and the new entry initialized. This prevents fatal error handling from reading past the buffer if reallocating it triggers an OOM bailout.

This zend_bailout path is also present in 8.5, the patch there would look like this?

-               EG(num_errors)++;
-               EG(errors) = erealloc(EG(errors), sizeof(zend_error_info*) * EG(num_errors));
-               EG(errors)[EG(num_errors)-1] = info;
+               uint32_t new_num_errors = EG(num_errors) + 1;
+               EG(errors) = erealloc(EG(errors), sizeof(zend_error_info*) * new_num_errors);
+               EG(errors)[EG(num_errors)] = info;
+               EG(num_errors) = new_num_errors;

---

If this fix is accepted, should I target 8.5 with a different PR, or what's the best workflow for changes that don't cleanly upmerge? I'm inexperienced in this regard.

---

Edit: Retargeting to 8.5 as @DanielEScherzer suggested

Changes for the master upmerge would be:

diff --git a/Zend/zend.c b/Zend/zend.c
index 07692db85196..e921bed4b812 100644
--- a/Zend/zend.c
+++ b/Zend/zend.c
@@ -1487,13 +1487,14 @@ ZEND_API ZEND_COLD void zend_error_zstr_at(
 		info->lineno = error_lineno;
 		info->filename = zend_string_copy(error_filename);
 		info->message = zend_string_copy(message);
-		EG(errors).size++;
-		if (EG(errors).size > EG(errors).capacity) {
+		uint32_t new_size = EG(errors).size + 1;
+		if (new_size > EG(errors).capacity) {
 			uint32_t capacity = EG(errors).capacity ? EG(errors).capacity + (EG(errors).capacity >> 1) : 2;
 			EG(errors).errors = erealloc(EG(errors).errors, sizeof(zend_error_info *) * capacity);
 			EG(errors).capacity = capacity;
 		}
-		EG(errors).errors[EG(errors).size - 1] = info;
+		EG(errors).errors[EG(errors).size] = info;
+		EG(errors).size = new_size;
 
 		/* Do not process non-fatal recorded error */
 		if (!(type & E_FATAL_ERRORS) || (type & E_DONT_BAIL)) {

[DanielEScherzer]

If this fix is accepted, should I target 8.5 with a different PR, or what's the best workflow for changes that don't cleanly upmerge? I'm inexperienced in this regard.

Generally you send the PR against the older branch. Good to know that this doesn't cleanly upmerge - if you want to switch this to target 8.5 with the changes for that branch, when upmerging you can just use edorian@5b36391 as the version for master

[edorian]

Well, that added a ton of people to the reviewers list. Sorry for the noise

[LamentXU123]

@edorian I did this too before. Github's UI sucks. When you change the "base" on the website though editing the title, it only change the base you're merging and won't rebase for you automatically so it generates millions of diff. This is indeed very misleading.

A suggestion is to draft the PR when doing "dangerous" stuff like changing the base through Github UI so it won't request review for you automatically :)

[edorian]

Thank you @LamentXU123, I'll do that next time

[LamentXU123]

That being said, I think this looks good. But since I have no experience in Zend engine I think other's review is way more persuasive and important.