Skip to content
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.

Sign up for GitHub

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

Jump to bottom

avoid undefined behaviour from downcasting ptr implentation #5

Merged
merged 2 commits into from
Jun 28, 2024
Merged

avoid undefined behaviour from downcasting ptr implentation #5

merged 2 commits into from
Jun 28, 2024

Conversation

gulafaran
Copy link
Contributor

errors can be seen with UBSAN in hyprland like

/usr/include/hyprutils/memory/SharedPtr.hpp:237:84: runtime error: member access within address 0x5030000e0710 which does not point to an object of type 'CSharedPointer_::impl<IKeyboard>'
0x5030000e0710: note: object is of type 'Hyprutils::Memory::CSharedPointer_::impl<CKeyboard>'
 00 00 00 00  10 c9 36 5e 55 55 00 00  02 00 00 00 02 00 00 00  80 fb 04 00 90 51 00 00  00 be be be
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'Hyprutils::Memory::CSharedPointer_::impl<CKeyboard>'
    #0 0x555559ff09f5 in Hyprutils::Memory::CSharedPointer<IKeyboard>::get() const /usr/include/hyprutils/memory/SharedPtr.hpp:237:84
    #1 0x555559fef658 in Hyprutils::Memory::CSharedPointer<IKeyboard>::operator->() const /usr/include/hyprutils/memory/SharedPtr.hpp:224:24
   

/usr/include/hyprutils/memory/WeakPtr.hpp:150:37: runtime error: downcast of address 0x5030000e0710 which does not point to an object of type 'CSharedPointer_::impl<IKeyboard>'
0x5030000e0710: note: object is of type 'Hyprutils::Memory::CSharedPointer_::impl<CKeyboard>'
 00 00 00 00  10 15 36 5e 55 55 00 00  03 00 00 00 03 00 00 00  80 fb 04 00 90 51 00 00  00 be be be
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'Hyprutils::Memory::CSharedPointer_::impl<CKeyboard>'
    #0 0x55555a58ae45 in Hyprutils::Memory::CWeakPointer<IKeyboard>::get() const /usr/include/hyprutils/memory/WeakPtr.hpp:150:37
    #1 0x55555a5842a8 in Hyprutils::Memory::CWeakPointer<IKeyboard>::operator->() const /usr/include/hyprutils/memory/WeakPtr.hpp:154:24

and the reason is because CKeyboard is a derived class from the base IKeyboard so the underlaying ptr can be casted but the implentation stored in the ptrs is not related. so add a getter and only cast the underlaying ptr.

also change C style casts in favour of reinterpret_cast.

gulafaran added 2 commits June 28, 2024 12:09
@gulafaran
sharedptr: dont downcast ptr implentation
4c10622 
avoid runtime errors of wrong downcasts by adding a underlaying ptr data
getter and only cast the data, downcasting the implentation type isnt
inherited from eachother, while the intention was do upcast/downcast
a derived class to a base class. found with UBSAN "runtime error:
downcast of address which does not point to an object of type
"CSharedPointer_::impl<IKeyboard>" note: object is of type
"Hyprutils::Memory::CSharedPointer_::impl<CKeyboard>"

also make dataNonNull check against != nullptr.
@gulafaran
sharedptr: use reinterpret_cast instead of c style
de8523a 
make it more type safe, C style casts tries every single one until one
works, or not. compilers also produces better warnings/errors when using
c++ casts in favour of C style ones.
vaxerski
vaxerski approved these changes Jun 28, 2024
View reviewed changes
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@vaxerski vaxerski merged commit 7e1b661 into hyprwm:main Jun 28, 2024
4 checks passed
@vaxerski vaxerski added the ABI Breaks the ABI label Jun 28, 2024
@vaxerski
Copy link
Member

vaxerski commented Jun 28, 2024 •
edited
Loading

or does it break the ABI? I think it does.

edit: yeah newer will expect a vtable entry for get

@gulafaran
Copy link
Contributor Author

or does it break the ABI? I think it does.

edit: yeah newer will expect a vtable entry for get

Yeah , Adding a virtual function breaks ABI because it changes the layout (and possibly the order) of entries in the vtable

@vaxerski
Copy link
Member

I don't think it changes the order, arent vtable indices just ordered the way they are in the src?

@gulafaran
Copy link
Contributor Author

I don't think it changes the order, arent vtable indices just ordered the way they are in the src?

havent digged that deep into vtables but googling it a bit the order is a bit up to the compiler but mostly should stay the same when it comes to adding new virtual functions at the end of the class, But we still have the derived impl class, so i suspect the addition of the function could change the order depending on how the vtable is layed out there?

i mean

base::foo()
base::foo2()
base::NEWFOO()

thats fine, order sticks.

but what about the derived?

base::foo()
base::foo2()
base::NEWFOO()
derived::bar()
derived::bar2()

derived::bar() and derived::bar2() is now moved down a step.

and isnt the size of the vtable now also potentially increased breaking ABI further?
dont take my words for fact im a mere apprentice when it comes to stuff like this heh

@vaxerski
Copy link
Member

ABI is broken and thats what really matters (I'll have to bump sover at release)

the rest was me just being curious

@gulafaran
Copy link
Contributor Author

ABI is broken and thats what really matters (I'll have to bump sover at release)

the rest was me just being curious

Yeah you made me curious aswell 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaxerski vaxerski vaxerski approved these changes

Assignees
No one assigned
Labels
ABI Breaks the ABI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gulafaran @vaxerski