Skip to content

Conversation

@matklad
Copy link
Contributor

@matklad matklad commented Aug 19, 2023

serde 1.0.172 and up rely on opaque non-reproducible binary blobs to function, explicitly not providing a library-level opt-out.

This is problematic for two reasons:

  • directly, unauditable binary blobs are a security issue.
  • indirectly, it becomes much harder to predict future behaviors of the crate.

As such, I am willing to go on a limb here and forbid building rust-analyzer with those versions of serde. Normally, my philosophy is to defer the choice to the end user, but it's also a design constraint of rust-analyzer that we don't run random binaries downloaded from the internet without explicit user's concent.

Concretely, this upper-bounds serde for both rust-analyzer workspace, as well as the lsp-server lib.

See serde-rs/serde#2538 for wider context.

serde 1.0.172 and up rely on opaque non-reproducible binary blobs to
function, explicitly not providing a library-level opt-out.

This is problematic for two reasons:

- directly, unauditable binary blobs are a security issue.
- indirectly, it becomes much harder to predict future behaviors of the
  crate.

As such, I am willing to go on a limb here and forbid building
rust-analyzer with those versions of serde. Normally, my philosophy is
to defer the choice to the end user, but it's also a design constraint
of rust-analyzer that we don't run random binaries downloaded from the
internet without explicit user's concent.

Concretely, this upper-bounds serde for both rust-analyzer workspace, as
well as the lsp-server lib.

See serde-rs/serde#2538 for wider context.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2023
Copy link
Member

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

I don't know for how long we can keep it like this, but sure, let's see what happens.

Too bad, though 😞.

@lnicola
Copy link
Member

lnicola commented Aug 19, 2023

I didn't look closely, can we build our own version of the blob and run it instead? EDIT: doesn't seem like it.

@Veykril
Copy link
Member

Veykril commented Aug 19, 2023

I doubt pinning the version will cause us problems given most people seem to do the same currently.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 19, 2023

📌 Commit 6c46b98 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 19, 2023

⌛ Testing commit 6c46b98 with merge 904b326...

@matklad
Copy link
Contributor Author

matklad commented Aug 19, 2023

Practically, I would thing that just sticking to 171 should be fine, I don’t thing we need latest and greatest serde.

for compile-time, the bigger problem for us is not that serde is slow to compile, but that it generates a loooot of code.

Long term, my plan always was to remove serde in favor of directly generateing slow-but-fast—to-compile code from LSP meta model

@bors
Copy link
Contributor

bors commented Aug 19, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 904b326 to master...

@bors bors merged commit 904b326 into rust-lang:master Aug 19, 2023
@matklad matklad deleted the 🪄deblobify branch August 19, 2023 14:26
azdavis added a commit to azdavis/millet that referenced this pull request Aug 19, 2023
Notably this pins serde at 1.0.171, see context:

rust-lang/rust-analyzer#15482
@matklad
Copy link
Contributor Author

matklad commented Aug 21, 2023

Learned about serde-rs/json#745. On a quick look, that sort of thing could be a major win for us. Also, serde-rs/serde#2250 (review) has a lot of valuable context.

@lnicola
Copy link
Member

lnicola commented Aug 21, 2023

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

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants