Skip to content

Avoid memcpy of invalid pointer #403

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 2 commits into from
Apr 22, 2025
Merged

Conversation

MichaelChirico
Copy link
Contributor

See r-lib/vctrs#1968 for more discussion

@jeroen
Copy link
Owner

jeroen commented Apr 22, 2025

Thanks. How is RAW_RO different from RAW ?

@MichaelChirico
Copy link
Contributor Author

It returns the const pointer, which matches the signature of memcpy, so should be ever-so-slightly preferable (in general, no specific relation to the invalid pointers issue):

https://github.com/r-devel/r-svn/blob/1c8173fa21cb59afe3a95ff8c55d77aba2743481/src/include/Defn.h#L423-L430

https://www.geeksforgeeks.org/memcpy-in-cc/

@jeroen
Copy link
Owner

jeroen commented Apr 22, 2025

OK thanks. Btw did you actually experience a problem or is this mostly precaution?

@jeroen jeroen merged commit 296292c into jeroen:master Apr 22, 2025
19 checks passed
@MichaelChirico MichaelChirico deleted the patch-2 branch April 22, 2025 18:09
@MichaelChirico
Copy link
Contributor Author

Hm, mix of both :)

I haven't experienced any problem from this specific {curl} code, but our compiler does segfault when encountering the RAW() [and friends] output on R>=4.5.0, i.e. I've the risk of invalid/undefined behavior access is not purely theoretical.

So if it's possible to reach this code with sensible (&public API) inputs, that segfault can happen. I just can't judge without a much more careful reading if there are other checks set up elsewhere that would prevent a 0-length memcpy() in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants