Skip to content

py/objtype: Validate super() arguments. #12918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Conversation

stinos
Copy link
Contributor

@stinos stinos commented Nov 8, 2023

Fixes issue #12830 i.e. various null dereferencing and out of bounds access because super_attr assumes the held obj is effectively an object of the held type, which is now verified. Modest code size increase, but could always wrap it in MICROPY_CPYTHON_COMPAT or so?

Also happens to fix the first poc in #12605 but see comments there: not really sufficient.

Copy link

github-actions bot commented Nov 8, 2023

Code size report:

   bare-arm:    +8 +0.014% 
minimal x86:   +16 +0.009% 
   unix x64:   +32 +0.004% standard
      stm32:   +16 +0.004% PYBV10
     mimxrt:    +8 +0.002% TEENSY40
        rp2:    +8 +0.001% RPI_PICO_W
       samd:    +8 +0.003% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@dlech
Copy link
Contributor

dlech commented Nov 8, 2023

Does this still work when super() is used in the __init__ of a function derived from a "native" type?

If I'm remembering correctly, there are some complications with this, like those discussed in #9997.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.43%. Comparing base (d1bf0ee) to head (093d0c0).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12918   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files         161      161           
  Lines       21277    21279    +2     
=======================================
+ Hits        20944    20946    +2     
  Misses        333      333           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stinos
Copy link
Contributor Author

stinos commented Nov 8, 2023

Should be the case yes, because for

class A(str):
    def __init__(self):
        super().__init__()

A()

the new code (i.e. super_make_new) isn't actually called, and calling it by super(str, A()) and super(str, A) the new version does identify correctly that it's proper usage; but do you have a more concrete usecase to test maybe?

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Nov 9, 2023
@dlech
Copy link
Contributor

dlech commented Nov 18, 2023

the new code (i.e. super_make_new) isn't actually called,

Ah, this is what I was wondering about. So, yeah, not a problem.

but do you have a more concrete usecase to test maybe?

Not related to my previous comments, but it looks like we don't have any tests that call super(A, self).__init__() which seems like a normal thing someone might try to do.

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

This fixes various null dereferencing and out-of-bounds access because
super_attr assumes the held obj is effectively an object of the held type,
which is now verified.

Fixes issue micropython#12830.

Signed-off-by: stijn <stijn@ignitron.net>
@dpgeorge
Copy link
Member

Rebased on master and tweaked the implementation to reduce code size (calling mp_obj_get_type unconditionally once, instead of mp_obj_is_type and then conditionally mp_obj_get_type).

@dpgeorge dpgeorge merged commit 093d0c0 into micropython:master Jul 25, 2024
63 checks passed
@stinos stinos deleted the 12830fix branch October 7, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants