py: Use a wrapper to explicitly check self argument of builtin methods. #1340
Conversation
Previous to this patch a call such as list.append(1, 2) would lead to a seg fault. This is because list.append is a builtin method and the first argument to such methods is always assumed to have the correct type. Now, when a builtin method is extracted like this it is wrapped in a checker object which checks the the type of the first argument before calling the builtin function. See issue micropython#1216.
|
Well, I guess it's a good compromise - don't add anything to the fast path, when method is used in OO manner, at the price of spending heap for for a much rare case of using it functional-style. But let's make it optional, to not grow size of minimal ports? |
|
The code costs 196 bytes on Thumb2. I can reduce that to 164 bytes if I make the error message contain no textual message and so just be a TypeError(). I would rather do this and keep it compiled in always than make it optional. As far as I know, list.append(1,2) is the only way to crash uPy (apart from using uctypes with random addresses) and it would be nice to plug this hole. Note that in all the tests that we have this code was hit only once (ie a checked_fun was created), and that was when testing for the existence of list.split_lines. So I guess such uses of this are rare. Any way that fixes this bug but doesn't use the heap would require a check for each and every call to see if the target function needs to have its first arg type checked. That would impact performance, and there would likely be many bytes of code required to do the type check (since one has to somehow do a search to find the correct type to check against). |
Yes, sure, it must be plugged. But it's certainly not the only and not the last issue. If uPy sees real usage, we'll get a lot of reports for corner cases.
And that's why I'd like to have it #ifdef'able. It's well-known that plugging each and every corner case may take a lot of checks, so would be nice to establish approach to that right away. A build with a check like that disabled would be able to run only trusted static code, but then it's pretty no-nonsense usecase. |
|
Ok, I'll add a #if. How about MICROPY_GUARD_CORNER_CASES? or CHECK_CORNER_CASES? |
|
If you don't feel to name it like MICROPY_BUILTIN_METHOD_CHECK_SELF, then let's just use MICROPY_CPYTHON_COMPAT until someone who gets a specific usecase proposes name like the above. Adding forward-looking generic stuff doesn't really help anyone. (Having bunch of that "generic high-level" stuff in axtls, that's what makes any "embedded" port of it dirty.) |
But we have a case right now. I don't see that it's useful to put each check into their own category. You can either afford the bytes and want to protect against all these cases, or not. All crashes are equal and there is no sense in trying to distinguish between them to say one is more important than another to protect against.
It's not a compatibility question, it's a question of making the code robust. |
|
But anyway, we can use MICROPY_BUILTIN_METHOD_CHECK_SELF for now, and if another such check comes along then we should merge them into the same category. After all, that was the reasoning behind MICROPY_CPYTHON_COMPAT and MICROPY_OPT_CACHE_MAP_LOOKUP_IN_BYTECODE. |
Well I shared my concern that naming things not what they are leads to confusion (only fortified after looking at another project from outside). If you don't think it applies here, sure, that name is ok. |
|
Ok, configurable by MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG, enabled by default, disabled for bare-arm, minimal and unix/minimal. Merge in 06593fb. |
Previous to this patch a call such as list.append(1, 2) would lead to a
seg fault. This is because list.append is a builtin method and the first
argument to such methods is always assumed to have the correct type.
Now, when a builtin method is extracted like this it is wrapped in a
checker object which checks the the type of the first argument before
calling the builtin function.
See issue #1216.