Skip to content

DiagnosticTextWriter panics when writing a marked value #737

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

Open
wata727 opened this issue Mar 8, 2025 · 0 comments · May be fixed by #739
Open

DiagnosticTextWriter panics when writing a marked value #737

wata727 opened this issue Mar 8, 2025 · 0 comments · May be fixed by #739
Labels

Comments

@wata727
Copy link
Contributor

wata727 commented Mar 8, 2025

See also terraform-linters/tflint#2243

The DiagnosticTextWriter will panic when it writes a diagnostic whose expression has a marked value. The reproducible code is below:

package main

import (
        "os"

        "github.com/hashicorp/hcl/v2"
        "github.com/hashicorp/hcl/v2/hclsyntax"
        "github.com/zclconf/go-cty/cty"
)

func main() {
        file, diags := hclsyntax.ParseConfig([]byte("foo = bar"), "main.tf", hcl.InitialPos)
        if diags.HasErrors() {
                panic(diags)
        }
        attrs, diags := file.Body.JustAttributes()
        if diags.HasErrors() {
                panic(diags)
        }
        expr := attrs["foo"].Expr
        ctx := &hcl.EvalContext{Variables: map[string]cty.Value{"bar": cty.StringVal("baz").Mark("x")}}

        diag := &hcl.Diagnostic{
                Severity:    hcl.DiagError,
                Summary:     "something went wrong",
                Subject:     expr.Range().Ptr(),
                Expression:  expr,
                EvalContext: ctx,
        }

        dwr := hcl.NewDiagnosticTextWriter(os.Stdout, map[string]*hcl.File{"main.tf": file}, 40, true)
        dwr.WriteDiagnostic(diag)
}
$ go run main.go
Error: something went wrong

  on main.tf line 1:
   1: foo = bar

panic: value is marked, so must be unmarked first

goroutine 1 [running]:
github.com/zclconf/go-cty/cty.Value.assertUnmarked(...)
        /go/pkg/mod/github.com/zclconf/[email protected]/cty/marks.go:141
github.com/zclconf/go-cty/cty.Value.AsString({{{0x67dfb0?, 0xc00009c00c?}}, {0x605d40?, 0xc0000b45a0?}})
        /go/pkg/mod/github.com/zclconf/[email protected]/cty/value_ops.go:1385 +0x47
github.com/hashicorp/hcl/v2.(*diagnosticTextWriter).valueStr(0x67dfb0?, {{{0x67dfb0?, 0xc00009c00c?}}, {0x605d40?, 0xc0000b45a0?}})
        /go/pkg/mod/github.com/hashicorp/hcl/[email protected]/diagnostic_text.go:274 +0x16c
github.com/hashicorp/hcl/v2.(*diagnosticTextWriter).WriteDiagnostic(0xc000093e70, 0xc000093e98)
        /go/pkg/mod/github.com/hashicorp/hcl/[email protected]/diagnostic_text.go:174 +0xc26
main.main()
        /workspaces/hcl/work/main.go:32 +0x44f
exit status 2
go.mod
module example.com/m

go 1.23.4

require github.com/hashicorp/hcl/v2 v2.23.0

require (
        github.com/agext/levenshtein v1.2.1 // indirect
        github.com/apparentlymart/go-textseg/v13 v13.0.0 // indirect
        github.com/apparentlymart/go-textseg/v15 v15.0.0 // indirect
        github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7 // indirect
        github.com/zclconf/go-cty v1.13.0 // indirect
        golang.org/x/mod v0.8.0 // indirect
        golang.org/x/sys v0.5.0 // indirect
        golang.org/x/text v0.11.0 // indirect
        golang.org/x/tools v0.6.0 // indirect
)
wata727 added a commit to wata727/hcl that referenced this issue Mar 8, 2025
Fixes hashicorp#737

When the `DiagnosticTextWriter` writes out a diagnostic,
if the diagnostic has an Expression and EvalContext,
it will also write out the evaluation context
in which the diagnostic occurred.

The `cty.Value` of the variable relevant to the expression
will be rendered as a string by `valueStr`, but this does not
take into account marked values and will cause a panic by
`val.AsString()`, `val.LengthInt'()`, etc.

This change prevents panics by always returning "(marked value)"
for marked values in `valueStr`. Whether this is always
appropriate is debatable, but it is better than
the current situation, which causes panics.
wata727 added a commit to wata727/hcl that referenced this issue Mar 8, 2025
Fixes hashicorp#737

When the `DiagnosticTextWriter` writes out a diagnostic,
if the diagnostic has an Expression and EvalContext,
it will also write out the evaluation context
in which the diagnostic occurred.

The `cty.Value` of the variable relevant to the expression
will be rendered as a string by `valueStr`, but this does not
take into account marked values and will cause a panic by
`val.AsString()`, `val.LengthInt()`, etc.

This change prevents panics by always returning "(marked value)"
for marked values in `valueStr`. Whether this is always
appropriate is debatable, but it is better than
the current situation, which causes panics.
wata727 added a commit to terraform-linters/tflint that referenced this issue Mar 10, 2025
Fixes #2243
See also hashicorp/hcl#737

The `hcl.DiagnosticTextWriter` has an issue where it will panic
when writing out a diagnostic that has an expression containing
a marked value.

TFLint can run into this issue when expanding the `for_each`
meta-argument with a sensitive value. If the `for_each` is not
iterable (it is not iterable if marked), a diagnostic containing
the expression context of the `for_each` is created and written
to stderr by the `DiagnosticTextWriter`.

Hopefully this will be fixed in upstream, but here's a workaround
to prevent the panic by excluding expression context from the
diagnostic when the `for_each` is marked.

By the way, other cases that include expression context in
diagnostics do not have the same issue, so no action is required.

- `for_each` is marked in dynamic blocks
  - The value is always unmarked, and non-iterable cases are rarer than those in meta-arguments
- dynamic block labels
  - Dynamic block labels are rarely used in Terraform
- `count` in meta-arguments
  - The value is always unmarked
@crw crw added the bug label Mar 10, 2025
wata727 added a commit to wata727/hcl that referenced this issue Mar 13, 2025
Fixes hashicorp#737
Follow up of hashicorp#738

When the `DiagnosticTextWriter` writes out a diagnostic,
if the diagnostic has an Expression and EvalContext,
it will also write out the evaluation context
in which the diagnostic occurred.

The `cty.Value` of the variable relevant to the expression
will be rendered as a string by `valueStr`, but calls to
`AsString` or `LengthInt` here will panic on marked values.

It is not clear how HCL should represent such marks,
so we skip the marked values from the additional context
as a best effort to be a general-purpose diagnostic writer,
and can consider how to output them later if needed.
wata727 added a commit to wata727/hcl that referenced this issue Mar 13, 2025
Fixes hashicorp#737
Follow up of hashicorp#738

When the `DiagnosticTextWriter` writes out a diagnostic,
if the diagnostic has an Expression and EvalContext,
it will also write out the evaluation context
in which the diagnostic occurred.

The `cty.Value` of the variable relevant to the expression
will be rendered as a string by `valueStr`, but calls to
`AsString` or `LengthInt` here will panic on marked values.

It is not clear how HCL should represent such marks,
so we skip the marked values from the additional context
as a general-purpose diagnostic writer,
and can consider how to output them later if needed.
wata727 added a commit to wata727/hcl that referenced this issue Mar 13, 2025
Fixes hashicorp#737
Follow up of hashicorp#738

When the `DiagnosticTextWriter` writes out a diagnostic,
if the diagnostic has an Expression and EvalContext,
it will also write out the evaluation context
in which the diagnostic occurred.

The `cty.Value` of the variable relevant to the expression
will be rendered as a string by `valueStr`, but calls to
`AsString` or `LengthInt` here will panic on marked values.

It is not clear how HCL should represent such marks,
so we skip the marked values from the additional context
as a general-purpose diagnostic writer.
@wata727 wata727 linked a pull request Mar 13, 2025 that will close this issue
wata727 added a commit to terraform-linters/tflint that referenced this issue Mar 28, 2025
Fixes #2243
See also hashicorp/hcl#737

The `hcl.DiagnosticTextWriter` has an issue where it will panic
when writing out a diagnostic that has an expression containing
a marked value.

TFLint can run into this issue when expanding the `for_each`
meta-argument with a sensitive value. If the `for_each` is not
iterable (it is not iterable if marked), a diagnostic containing
the expression context of the `for_each` is created and written
to stderr by the `DiagnosticTextWriter`.

Hopefully this will be fixed in upstream, but here's a workaround
to prevent the panic by excluding expression context from the
diagnostic when the `for_each` is marked.

By the way, other cases that include expression context in
diagnostics do not have the same issue, so no action is required.

- `for_each` is marked in dynamic blocks
  - The value is always unmarked, and non-iterable cases are rarer than those in meta-arguments
- dynamic block labels
  - Dynamic block labels are rarely used in Terraform
- `count` in meta-arguments
  - The value is always unmarked
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants