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.

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

[R] Sanitize R metadata #42143

Closed
nealrichardson opened this issue Jun 13, 2024 · 0 comments
Closed

[R] Sanitize R metadata #42143

nealrichardson opened this issue Jun 13, 2024 · 0 comments

Comments

@nealrichardson
Copy link
Member

Describe the enhancement requested

Backfilling an issue for #41969

Component(s)

R

nealrichardson added a commit that referenced this issue Jun 14, 2024
### Rationale for this change

`arrow` uses R `serialize()`/`unserialize()` to store additional
metadata in the Arrow schema. This PR adds some extra checking and
sanitizing in order to make the reading of this metadata robust to data
of unknown provenance.

### What changes are included in this PR?

* When writing metadata, we strip out all but simple types: strings,
numbers, boolean, lists, etc. Objects of other types, such as
environments, external pointers, and other language types, are removed.
* When reading metadata, the same filter is applied. If there are types
that are not in the allowlist, one of two things happen. By default,
they are removed with a warning. If you set
`options(arrow.unsafe_metadata = TRUE)`, the full metadata including
disallowed types is returned, also with a warning. This option is an
escape hatch in case we are too strict with dropping types when reading
files produced by older versions of the package that did not filter them
out.
* `unserialize()` is called in a way that prevents promises contained in
the data from being automatically invoked. This technique works on all
versions of R: it is not dependent on the patch for RDS reading that was
included in 4.4.
* Other sanity checking to be stricter about only reading back in
something of the form we wrote out: assert that the data is
ASCII-serialized, and if it is compressed, it is gzip, the same way we
do on serialization. It's not clear that it's necessary, but it's not
bad to be extra strict here.

### Are these changes tested?

Yes

### Are there any user-facing changes?

For most, no. But:

**This PR contains a "Critical Fix".** 

Without this patch, it is possible to construct an Arrow or Parquet file
that would contain code that would execute when the R metadata is
applied when converting to a data.frame. If you are using an older
version of the package and are reading data from a source you do not
trust, you can read into a `Table` and use its internal
`$to_data_frame()` method, like `read_parquet(..., as_data_frame =
FALSE)$to_data_frame()`. This should skip the reading of the R metadata.
* GitHub Issue: #42143
@nealrichardson nealrichardson added this to the 17.0.0 milestone Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant