Use common function for TypeError on illegal offset access #10544

Girgias · 2023-02-08T13:00:26Z

Follow-up on #10504

This also fixes some consistency issues

Currently, using CI to check the JIT changes are correct as I am getting weird errors (possibly due to ASAN/UBSAN)

Girgias · 2023-02-08T14:56:48Z

Would like to get opinions on the approach of using the BP_VAR_* constants.

Also, I've just used BP_VAR_R to get the default message, but my usages are probably wrong.

Opinions @arnaud-lb, @iluuu1994, @derickr ?

iluuu1994

I'm always in favor of improving error messages. It seems like this will be somewhat inconsistent though as some of these functions aren't aware of the BP and can be used for any of them, but propagating it just for the error message doesn't seem worth it.

ext/spl/spl_array.c

ext/opcache/jit/zend_jit_helpers.c

Zend/zend_vm_def.h

Zend/zend_execute.c

Girgias · 2023-02-09T15:47:12Z

Zend/tests/bug24773.phpt

@@ -6,7 +6,7 @@ Bug #24773 (unset() of integers treated as arrays causes a crash)
    unset($array["lvl1"]["lvl2"]["b"]);
 ?>
 --EXPECTF--
-Fatal error: Uncaught TypeError: Cannot access offset of type string on string in %s:%d
+Fatal error: Uncaught TypeError: Cannot access offset of type string in unset in %s:%d


I think this case shows we probably should improve the message somewhat. As this is inconsistent with trying to unset the offset of a string (which is banned in general). Maybe strings do need to be seperate :/

iluuu1994

I didn't go through all the changes this time because it doesn't seem the BPs have changed.

Zend/zend.c

iluuu1994 · 2023-05-31T15:34:43Z

ext/spl/spl_fixedarray.c

@@ -333,7 +333,8 @@ static zend_long spl_offset_convert_to_long(zval *offset) /* {{{ */
 			return Z_RES_HANDLE_P(offset);
 	}

-	zend_illegal_container_offset("SplFixedArray", offset, BP_VAR_R);
+	/* Use SplFixedArray name from the CE */
+	zend_illegal_container_offset(spl_ce_SplFixedArray->name, offset, BP_VAR_R);


To be fully correct this would need to get the class name passed from the caller.

I know, but that requires some more intrusive changes, which I want to do as a follow-up. As I first notices this being an issue with the ArrayObject case. But I think in both cases some places have hard coded the value of the class name.

dstogov · 2023-06-01T06:33:22Z

Zend/zend_execute.c

@@ -1651,7 +1631,7 @@ static zend_never_inline zend_long zend_check_string_offset(zval *dim, int type
 				}
 				return offset;
 			}
-			zend_illegal_string_offset(dim);
+			zend_illegal_container_offset(ZSTR_KNOWN(ZEND_STR_STRING), dim, type);


You replace the calls to specialized functions with the calls to a single universal one. This requires sending more arguments and therefore will increases the VM code size.

arnaud-lb

Looks good to me appart from other comments

arnaud-lb · 2023-06-03T10:49:48Z

Zend/tests/036.phpt

@@ -11,4 +11,4 @@ try {

 ?>
 --EXPECT--
-Cannot access offset of type object on array
+Cannot access offset of type Closure on array


Only a question, not a suggestion: What do you think of also specifying what is allowed? Array offset must be int or string, cannot access offset of type Closure

We can only really do this for arrays and strings :/ as objects that implement ArrayOffset can in theory support any type

dstogov

I suggest to split ``zend_illegal_container_offset() back to cheaper specialized functions at least for the VM handlers.

This merges all usages of emiting an offset TypeError into a new ZEND_API function zend_illegal_container_offset(const char* container, const zval *offset, int type); Where the container should represent the type on which the access is attempted (e.g. string, array) The offset zval that is used, where the error message will display its type The type of access, which should be a BP_VAR_* constant, to get special message for isset/empty/unset

Also pass called CE

Girgias · 2023-06-05T14:38:19Z

I suggest to split ``zend_illegal_container_offset() back to cheaper specialized functions at least for the VM handlers.

I hope what I've done is what you asked, should I do the same for the JIT VM handlers?

dstogov

In this state, the patch shouldn't make any harm, but I also don't see any benefits.
I'm indifferent.

github-actions bot added Category: Engine Extension: opcache Extension: spl Extension: standard labels Feb 8, 2023

Girgias force-pushed the offset-common-error-func branch from 7f87332 to f0d0e33 Compare February 8, 2023 13:44

Girgias marked this pull request as ready for review February 8, 2023 14:55

iluuu1994 reviewed Feb 8, 2023

View reviewed changes

Girgias force-pushed the offset-common-error-func branch from f0d0e33 to 93e78d4 Compare February 9, 2023 15:16

Girgias commented Feb 9, 2023

View reviewed changes

Girgias force-pushed the offset-common-error-func branch from 93e78d4 to 1d35b17 Compare May 31, 2023 15:11

Girgias requested review from dstogov and bukka as code owners May 31, 2023 15:11

Girgias requested a review from iluuu1994 May 31, 2023 15:17

iluuu1994 approved these changes May 31, 2023

View reviewed changes

dstogov reviewed Jun 1, 2023

View reviewed changes

arnaud-lb reviewed Jun 3, 2023

View reviewed changes

dstogov requested changes Jun 5, 2023

View reviewed changes

Girgias added 3 commits June 5, 2023 15:14

Use zend_string for container variable

9591a88

Also pass called CE

Consistent error messages for unsetting a string offset

d445bf6

Girgias force-pushed the offset-common-error-func branch from 1d35b17 to 381e865 Compare June 5, 2023 14:37

Girgias added 3 commits June 5, 2023 16:27

More consistent unset error message

38d3bfc

Use specialized VM functions again

4e58749

Pass access type again to string offset error function

c41efe0

Girgias force-pushed the offset-common-error-func branch from 381e865 to c41efe0 Compare June 5, 2023 15:34

dstogov reviewed Jun 5, 2023

View reviewed changes

Girgias merged commit 99fa740 into php:master Jun 6, 2023

Girgias deleted the offset-common-error-func branch June 6, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use common function for TypeError on illegal offset access #10544

Use common function for TypeError on illegal offset access #10544

Girgias commented Feb 8, 2023

Girgias commented Feb 8, 2023

iluuu1994 left a comment

Girgias Feb 9, 2023

iluuu1994 left a comment

iluuu1994 May 31, 2023

Girgias May 31, 2023

dstogov Jun 1, 2023

arnaud-lb left a comment

arnaud-lb Jun 3, 2023

Girgias Jun 5, 2023

dstogov left a comment

Girgias commented Jun 5, 2023

dstogov left a comment

Use common function for TypeError on illegal offset access #10544

Use common function for TypeError on illegal offset access #10544

Conversation

Girgias commented Feb 8, 2023

Girgias commented Feb 8, 2023

iluuu1994 left a comment

Choose a reason for hiding this comment

Girgias Feb 9, 2023

Choose a reason for hiding this comment

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 May 31, 2023

Choose a reason for hiding this comment

Girgias May 31, 2023

Choose a reason for hiding this comment

dstogov Jun 1, 2023

Choose a reason for hiding this comment

arnaud-lb left a comment

Choose a reason for hiding this comment

arnaud-lb Jun 3, 2023

Choose a reason for hiding this comment

Girgias Jun 5, 2023

Choose a reason for hiding this comment

dstogov left a comment

Choose a reason for hiding this comment

Girgias commented Jun 5, 2023

dstogov left a comment

Choose a reason for hiding this comment