-
Notifications
You must be signed in to change notification settings - Fork 817
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
Hopefully avoid panic in DescribeWorkflowExecution #5057
Hopefully avoid panic in DescribeWorkflowExecution #5057
Conversation
Status: executionState.Status, | ||
HistoryLength: mutableState.GetNextEventID() - common.FirstEventID, | ||
ExecutionTime: executionInfo.ExecutionTime, | ||
// Memo and SearchAttributes are set below | ||
AutoResetPoints: executionInfo.AutoResetPoints, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also make a deep copy here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
d345aef
to
a55eed2
Compare
for k, v := range source { | ||
metadata := make(map[string][]byte, len(v.GetMetadata())) | ||
for mk, mv := range v.GetMetadata() { | ||
metadata[mk] = mv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also want to copy byte slices because, even though another thread can't append to this or delete from it, they can modify its elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't happen in practice, I don't think we mutate any payload metadata or data directly anywhere, only replace the entire slice.
In any case it's safe in terms of serialization and would mitigate the panic since the slice's length won't change.
} | ||
target[k] = &commonpb.Payload{ | ||
Metadata: metadata, | ||
Data: v.GetData(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here
defer func() { weCtx.GetReleaseFn()(retError) }() | ||
|
||
mutableState := weCtx.GetMutableState() | ||
executionInfo := mutableState.GetExecutionInfo() | ||
executionState := mutableState.GetExecutionState() | ||
|
||
resetPoints := &workflowpb.ResetPoints{ | ||
Points: make([]*workflowpb.ResetPointInfo, 0, len(executionInfo.AutoResetPoints.GetPoints())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause an OOB exception. You need to remove the second argument.
a55eed2
to
122f052
Compare
**What changed?** Clone suspected mutating fields after a returning from DescribeWorkflowExecution. **Why?** Noticed panics in serialization of the `DescribeWorkflowExecutionResponse` object. **How did you test it?** No tests added, assuming that normal test coverage is enough. Haven't reproduced the issue, this is based on a hunch. **Potential risks** None that I can foresee. **Is hotfix candidate?** Yes.
**What changed?** Clone suspected mutating fields after a returning from DescribeWorkflowExecution. **Why?** Noticed panics in serialization of the `DescribeWorkflowExecutionResponse` object. **How did you test it?** No tests added, assuming that normal test coverage is enough. Haven't reproduced the issue, this is based on a hunch. **Potential risks** None that I can foresee. **Is hotfix candidate?** Yes.
What changed?
Clone suspected mutating fields after a returning from DescribeWorkflowExecution.
Why?
Noticed panics in serialization of the
DescribeWorkflowExecutionResponse
object.How did you test it?
No tests added, assuming that normal test coverage is enough.
Haven't reproduced the issue, this is based on a hunch.
Potential risks
None that I can foresee.
Is hotfix candidate?
Yes.