-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Avoid modification of user input data #1285
Conversation
Fixes #1277 ### Details **Node** - Ensure that user data given to public API functions is not internally modified (so clone it before using it). - Use proper `utils.clone<T>(xxx)` syntax everywhere. - Remove no longer needed functions in `ortc.ts` (unused since Flatbuffers migration). - Do not export unneeded functions in `ortc.ts` (keep them private).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert all Rust changes, they are not idiomatic and not necessary.
Rust APIs should clearly specify what they need and not more. In this example functions already receive owned value of rtp_parameters
that will be dropped at the end of the function because it will need an owned value and will be modifying it. All you're doing by cloning it is once more unnecessary allocation. If caller does have a spare value to give away, no extra allocation will be necessary with old API, if there isn't, the caller will have to do explicit clone. But getting value as an argument that you know will always be cloned is anti-pattern in Rust because it introduces unnecessary allocations that caller can't prevent.
Also it is not pretty to add cloned_
prefix or similar suffix to a variable in Rust. You can redefine variable name even with a different type if you need to in contrast to TypeScript.
Example:
Basically you are invalidating the whole purpose of this PR by arguing that the user should clone the data by himself if he doesn't want it to be potentially modified by mediasoup. I just don't get it. |
Not possible, ownership of |
Ok, that solves it. May I know how such a ownership transfer is declared in the code? |
If you specify a variable - you transfer ownership, if you want to give a reference to the data/borrow it, you use |
Rust changes reverted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust changes look fine to me 😂
Thanks, I had to investigate and learn a lot to write them properly. |
Fixes #1277
Details
Node
utils.clone<T>(xxx)
syntax everywhere.ortc.ts
(unused since Flatbuffers migration).ortc.ts
(keep them private).