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

Crash when a refined unknown string has too long a prefix #33464

Closed
apparentlymart opened this issue Jun 30, 2023 · 2 comments · Fixed by #33741
Closed

Crash when a refined unknown string has too long a prefix #33464

apparentlymart opened this issue Jun 30, 2023 · 2 comments · Fixed by #33741
Assignees
Labels
bug v1.6 Issues (primarily bugs) reported against v1.6 releases
Milestone

Comments

@apparentlymart
Copy link
Member

This is a proxy issue for hashicorp/hcl#616 which describes how HCL template expressions with particularly long known prefixes can produce an unknown value with a refinement so long that it gets rejected by the safety limits in cty's msgpack decoder. We will need to do something about this before the v1.6.0 freeze.

A short-term pragmatic fix would be to change HCL's own template implementation to truncate its own known string prefixes, since returning a shorter prefix just broadens the range and so would not invalidate the refinement.

A more robust answer would be for cty's msgpack implementation -- which is the one actually imposing this limit -- to include logic in its encoder to ensure that it never produces a message that its decoder would not accept. Again I think that means shortening the known string prefix so that the result is still refined but with not quite so tight a range. Serialization is already permitted to lose refinement detail -- at worst, to discard refinements altogether and only keep the unknown-ness -- so such behavior is okay in principle but may require some care in its exact implementation to avoid troubles with truncating into the middle of a multi-code-unit sequence such as a latin letter followed by a combining diacritic.

@apparentlymart apparentlymart added bug v1.6 Issues (primarily bugs) reported against v1.6 releases labels Jun 30, 2023
@apparentlymart apparentlymart added this to the v1.6 milestone Jun 30, 2023
@apparentlymart
Copy link
Member Author

apparentlymart commented Jul 7, 2023

We merged a tactical fix for the HCL template manifestation of this problem, but I think it's still worth a more general solution so this can't crop up in other situations.

Since it's cty's MessagePack decoder that's the one enforcing this limit, my favorite idea to address this is to change cty's MessagePack encoder so that it will truncate long string prefixes at serialization time, choosing a limit that has at least enough margin to allow encoding "not null" alongside it without the resulting refinements blob being greater than 1024 bytes. In practice it's probably worth making the limit shorter than strictly required because I don't expect it to make a significant difference to store a long prefix -- the string prefix refinements are primarily beneficial for short, fixed prefixes like ami- on an unknown AMI ID -- and so I think it's a better tradeoff to keep the serialized refinements relatively compact.

Even if I don't make a cty-level change before the v1.6.0 release, we should still make a new HCL release including the tactical fix and upgrade Terraform to use it.

Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug v1.6 Issues (primarily bugs) reported against v1.6 releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants