Keep a copy of block stream instead of raw pointer - #310
Conversation
IsaacWoods
left a comment
IsaacWoods
left a comment
There was a problem hiding this comment.
I think this is probably a reasonable change given the risk of use-after-free in the given examples. We can always do this and then explore options with less copies in the future.
I tried using borrows but it was leading to a horrible mess of lifetime specifiers and there were still a few areas of code I couldn't figure out how to make compile...
Yes, I think this is how it originally ended up being a raw pointer. Representing the lifetime of something that could be mapped from physical memory (DSDT/SSDTs) or a method that has been copied already is tricky.
Idle thought: with the split of PhysicalMappings in #345, one possibility is to treat a new PhysicalMapping more like an Arc than a Box (i.e. we reference count them and only free the underlying 'raw' mapping when no references exist. This would allow a MethodContext to own a mapping to the DSDT, and then could use that or a Vec in the case of a copied method. We could even let methods 'own' a mapping to a table and then index into that table for the method code? Worth exploring perhaps?
There was a problem hiding this comment.
This comment is now out of date, as the lifetime of the underlying data doesn't matter so much if we're copying it.
There was a problem hiding this comment.
Comment removed 😊
This mitigates the possibility of the pointed-to code stream being dropped, which would result in a use-after-free. Fixes rust-osdev#300. The use of a Vec isn't the most memory or CPU efficient. I tried using borrows but it was leading to a horrible mess of lifetime specifiers and there were still a few areas of code I couldn't figure out how to make compile...
2d33ba9 to
32d7f77
Compare
2 participants
This mitigates the possibility of the pointed-to code stream being dropped, which would result in a use-after-free.
Fixes #300.
The use of a Vec isn't the most memory or CPU efficient. I tried using borrows but it was leading to a horrible mess of lifetime specifiers and there were still a few areas of code I couldn't figure out how to make compile...
In #300 I suggested a total fix might involve "When doing a copy object op, create a new WrappedObject and insert it into the namespace with the same name, instead of replacing the contents of the destination."
I didn't do that because:
do_copy_objectdoesn't know about the names or namespace locations of objects and,