Fix phpGH-8289: Exceptions thrown within a yielded from iterator are …

…not rethrown into the generator This also fixes the fact that exception traces were not including the generator frame when thrown in a yielded from iterator.
imme-emosol · Apr 1, 2022 · 1364945 · 1364945
1 parent d82d62c
commit 1364945
Show file tree

Hide file tree

Showing 3 changed files with 110 additions and 81 deletions.
diff --git a/NEWS b/NEWS
@@ -7,6 +7,8 @@ PHP                                                                        NEWS
   . Fixed bug GH-8070 (memory leak of internal function attribute hash).
     (Tim Düsterhus)
   . Fixed bug GH-8160 (ZTS support on Alpine is broken). (Michael Voříšek)
+  . Fixed bug GH-8289 (Exceptions thrown within a yielded from iterator are
+    not rethrown into the generator). (Bob)
 
 - Filter:
   . Fixed signedness confusion in php_filter_validate_domain(). (cmb)
@@ -43,8 +45,6 @@ PHP                                                                        NEWS
 
 - Core:
   . Fixed Haiku ZTS build. (David Carlier)
-  . Fixed bug GH-8082 (op_arrays with temporary run_time_cache leak memory
-    when observed). (Bob)
 
 - GD:
   . Fixed libpng warning when loading interlaced images. (Brett)

diff --git a/Zend/tests/generators/gh8289.phpt b/Zend/tests/generators/gh8289.phpt
@@ -0,0 +1,34 @@
+--TEST--
+GH-8289 (Exceptions thrown within a yielded from iterator are not rethrown into the generator)
+--FILE--
+<?php
+
+function yieldFromIteratorGeneratorThrows() {
+	try {
+		yield from new class(new ArrayIterator([1, -2])) extends IteratorIterator {
+			public function key() {
+				if ($k = parent::key()) {
+					throw new Exception;
+				}
+				return $k;
+			}
+		};
+	} catch (Exception $e) {
+		echo "$e\n";
+		yield 2;
+	}
+}
+
+foreach (yieldFromIteratorGeneratorThrows() as $k => $v) {
+    var_dump($v);
+}
+
+?>
+--EXPECTF--
+int(1)
+Exception in %s:%d
+Stack trace:
+#0 %s(%d): IteratorIterator@anonymous->key()
+#1 %s(%d): yieldFromIteratorGeneratorThrows()
+#2 {main}
+int(2)
diff --git a/Zend/zend_generators.c b/Zend/zend_generators.c
@@ -637,22 +637,17 @@ static zend_result zend_generator_get_next_delegated_value(zend_generator *gener
 		if (iter->index++ > 0) {
 			iter->funcs->move_forward(iter);
 			if (UNEXPECTED(EG(exception) != NULL)) {
-				goto exception;
+				goto failure;
 			}
 		}
 
 		if (iter->funcs->valid(iter) == FAILURE) {
-			if (UNEXPECTED(EG(exception) != NULL)) {
-				goto exception;
-			}
 			/* reached end of iteration */
 			goto failure;
 		}
 
 		value = iter->funcs->get_current_data(iter);
-		if (UNEXPECTED(EG(exception) != NULL)) {
-			goto exception;
-		} else if (UNEXPECTED(!value)) {
+		if (UNEXPECTED(EG(exception) != NULL) || UNEXPECTED(!value)) {
 			goto failure;
 		}
 
@@ -664,17 +659,14 @@ static zend_result zend_generator_get_next_delegated_value(zend_generator *gener
 			iter->funcs->get_current_key(iter, &generator->key);
 			if (UNEXPECTED(EG(exception) != NULL)) {
 				ZVAL_UNDEF(&generator->key);
-				goto exception;
+				goto failure;
 			}
 		} else {
 			ZVAL_LONG(&generator->key, iter->index);
 		}
 	}
 	return SUCCESS;
 
-exception:
-	zend_generator_throw_exception(generator, NULL);
-
 failure:
 	zval_ptr_dtor(&generator->values);
 	ZVAL_UNDEF(&generator->values);
@@ -706,94 +698,97 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */
 	/* Drop the AT_FIRST_YIELD flag */
 	orig_generator->flags &= ~ZEND_GENERATOR_AT_FIRST_YIELD;
 
+	/* Backup executor globals */
+	zend_execute_data *original_execute_data = EG(current_execute_data);
+	uint32_t original_jit_trace_num = EG(jit_trace_num);
+
+	/* Set executor globals */
+	EG(current_execute_data) = generator->execute_data;
+	EG(jit_trace_num) = 0;
+
+	/* We want the backtrace to look as if the generator function was
+	 * called from whatever method we are current running (e.g. next()).
+	 * So we have to link generator call frame with caller call frame. */
+	if (generator == orig_generator) {
+		generator->execute_data->prev_execute_data = original_execute_data;
+	} else {
+		/* We need some execute_data placeholder in stacktrace to be replaced
+		 * by the real stack trace when needed */
+		generator->execute_data->prev_execute_data = &orig_generator->execute_fake;
+		orig_generator->execute_fake.prev_execute_data = original_execute_data;
+	}
+
+	/* Ensure this is run after executor_data swap to have a proper stack trace */
 	if (UNEXPECTED(!Z_ISUNDEF(generator->values))) {
 		if (EXPECTED(zend_generator_get_next_delegated_value(generator) == SUCCESS)) {
+			/* Restore executor globals */
+			EG(current_execute_data) = original_execute_data;
+			EG(jit_trace_num) = original_jit_trace_num;
+
 			orig_generator->flags &= ~ZEND_GENERATOR_DO_INIT;
 			return;
 		}
 		/* If there are no more deletegated values, resume the generator
 		 * after the "yield from" expression. */
 	}
 
-	{
-		/* Backup executor globals */
-		zend_execute_data *original_execute_data = EG(current_execute_data);
-		uint32_t original_jit_trace_num = EG(jit_trace_num);
-
-		/* Set executor globals */
-		EG(current_execute_data) = generator->execute_data;
-		EG(jit_trace_num) = 0;
-
-		/* We want the backtrace to look as if the generator function was
-		 * called from whatever method we are current running (e.g. next()).
-		 * So we have to link generator call frame with caller call frame. */
-		if (generator == orig_generator) {
-			generator->execute_data->prev_execute_data = original_execute_data;
-		} else {
-			/* We need some execute_data placeholder in stacktrace to be replaced
-			 * by the real stack trace when needed */
-			generator->execute_data->prev_execute_data = &orig_generator->execute_fake;
-			orig_generator->execute_fake.prev_execute_data = original_execute_data;
-		}
+	if (UNEXPECTED(generator->frozen_call_stack)) {
+		/* Restore frozen call-stack */
+		zend_generator_restore_call_stack(generator);
+	}
 
-		if (UNEXPECTED(generator->frozen_call_stack)) {
-			/* Restore frozen call-stack */
-			zend_generator_restore_call_stack(generator);
+	/* Resume execution */
+	generator->flags |= ZEND_GENERATOR_CURRENTLY_RUNNING;
+	if (!ZEND_OBSERVER_ENABLED) {
+		zend_execute_ex(generator->execute_data);
+	} else {
+		zend_observer_generator_resume(generator->execute_data);
+		zend_execute_ex(generator->execute_data);
+		if (generator->execute_data) {
+			/* On the final return, this will be called from ZEND_GENERATOR_RETURN */
+			zend_observer_fcall_end(generator->execute_data, &generator->value);
 		}
+	}
+	generator->flags &= ~ZEND_GENERATOR_CURRENTLY_RUNNING;
 
-		/* Resume execution */
-		generator->flags |= ZEND_GENERATOR_CURRENTLY_RUNNING;
-		if (!ZEND_OBSERVER_ENABLED) {
-			zend_execute_ex(generator->execute_data);
-		} else {
-			zend_observer_generator_resume(generator->execute_data);
-			zend_execute_ex(generator->execute_data);
-			if (generator->execute_data) {
-				/* On the final return, this will be called from ZEND_GENERATOR_RETURN */
-				zend_observer_fcall_end(generator->execute_data, &generator->value);
-			}
-		}
-		generator->flags &= ~ZEND_GENERATOR_CURRENTLY_RUNNING;
+	generator->frozen_call_stack = NULL;
+	if (EXPECTED(generator->execute_data) &&
+		UNEXPECTED(generator->execute_data->call)) {
+		/* Frize call-stack */
+		generator->frozen_call_stack = zend_generator_freeze_call_stack(generator->execute_data);
+	}
 
-		generator->frozen_call_stack = NULL;
-		if (EXPECTED(generator->execute_data) &&
-		    UNEXPECTED(generator->execute_data->call)) {
-			/* Frize call-stack */
-			generator->frozen_call_stack = zend_generator_freeze_call_stack(generator->execute_data);
-		}
+	/* Restore executor globals */
+	EG(current_execute_data) = original_execute_data;
+	EG(jit_trace_num) = original_jit_trace_num;
 
-		/* Restore executor globals */
-		EG(current_execute_data) = original_execute_data;
-		EG(jit_trace_num) = original_jit_trace_num;
-
-		/* If an exception was thrown in the generator we have to internally
-		 * rethrow it in the parent scope.
-		 * In case we did yield from, the Exception must be rethrown into
-		 * its calling frame (see above in if (check_yield_from). */
-		if (UNEXPECTED(EG(exception) != NULL)) {
-			if (generator == orig_generator) {
-				zend_generator_close(generator, 0);
-				if (!EG(current_execute_data)) {
-					zend_throw_exception_internal(NULL);
-				} else if (EG(current_execute_data)->func &&
-						ZEND_USER_CODE(EG(current_execute_data)->func->common.type)) {
-					zend_rethrow_exception(EG(current_execute_data));
-				}
-			} else {
-				generator = zend_generator_get_current(orig_generator);
-				zend_generator_throw_exception(generator, NULL);
-				orig_generator->flags &= ~ZEND_GENERATOR_DO_INIT;
-				goto try_again;
+	/* If an exception was thrown in the generator we have to internally
+	 * rethrow it in the parent scope.
+	 * In case we did yield from, the Exception must be rethrown into
+	 * its calling frame (see above in if (check_yield_from). */
+	if (UNEXPECTED(EG(exception) != NULL)) {
+		if (generator == orig_generator) {
+			zend_generator_close(generator, 0);
+			if (!EG(current_execute_data)) {
+				zend_throw_exception_internal(NULL);
+			} else if (EG(current_execute_data)->func &&
+					ZEND_USER_CODE(EG(current_execute_data)->func->common.type)) {
+				zend_rethrow_exception(EG(current_execute_data));
 			}
-		}
-
-		/* yield from was used, try another resume. */
-		if (UNEXPECTED((generator != orig_generator && !Z_ISUNDEF(generator->retval)) || (generator->execute_data && (generator->execute_data->opline - 1)->opcode == ZEND_YIELD_FROM))) {
+		} else {
 			generator = zend_generator_get_current(orig_generator);
+			zend_generator_throw_exception(generator, NULL);
+			orig_generator->flags &= ~ZEND_GENERATOR_DO_INIT;
 			goto try_again;
 		}
 	}
 
+	/* yield from was used, try another resume. */
+	if (UNEXPECTED((generator != orig_generator && !Z_ISUNDEF(generator->retval)) || (generator->execute_data && (generator->execute_data->opline - 1)->opcode == ZEND_YIELD_FROM))) {
+		generator = zend_generator_get_current(orig_generator);
+		goto try_again;
+	}
+
 	orig_generator->flags &= ~ZEND_GENERATOR_DO_INIT;
 }
 /* }}} */