fix: fix data race in echo provisioner #17142
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes coder/internal#546
Closes coder/internal#543
Fixes a data race in the echo provisioner
What happened
If you're curious, two things:
I was under the assumption that the incoming responses would always be under the ownership of the provisioner (no need to worry about synchronization or locking) because it would be coming from freshly decoded protobuf messages. Apparently this is very wrong, because we just call the echo provisioner functions directly using protobuf types in a bunch of tests. Serialization/deserialization doesn't happen anywhere in the process, we're just passing pointers around.
The incoming responses are a mixture of values which could be interpreted as "transferring ownership", and a few global/"static" values that we have around for common scenarios. Mutating the unique "owned" responses is fine, but mutating those global helpers is where the data race was occurring. The code was checking to see if the plan was "empty" (as in
len(plan.Plan) == 0
) and then replacing it with a "valid empty" plan (as in[]byte("{}")
, empty JSON object). The problem with this is that there should not be a valid plan ifplan.Error
is set. The "valid empty" plan was already added as a field to thePlanComplete
static helper, but was intentionally omitted from a few other helpers that haveError
values.So with all that context, the fix is to just not mutate
plan.Plan
ifplan.Error
is set.Also, since in production environments the data will actually be owned by this code, freshly deserialized from a protobuf message, I don't think there's any way this write could affect a real deployment.