Skip to content

Conversation

shogo82148
Copy link
Contributor

Issue #, if available:

the safeMarshal function might return invalid JSON if err.Error() contains special characters of JSON.

func safeMarshal(v interface{}) []byte {
payload, err := json.Marshal(v)
if err != nil {
return []byte(fmt.Sprintf(serializationErrorFormat, err.Error()))
}
return payload
}

Description of changes:

use json.Marshal instead of fmt.Sprintf.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2020

Codecov Report

Merging #326 into master will decrease coverage by 2.22%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
- Coverage   74.46%   72.23%   -2.23%     
==========================================
  Files          18       18              
  Lines         834      724     -110     
==========================================
- Hits          621      523      -98     
+ Misses        150      136      -14     
- Partials       63       65       +2     
Impacted Files Coverage Δ
lambda/invoke_loop.go 72.00% <50.00%> (-2.55%) ⬇️
lambda/errors.go 77.27% <0.00%> (-17.97%) ⬇️
lambda/rpc.go 55.55% <0.00%> (-8.09%) ⬇️
events/epoch_time.go 77.77% <0.00%> (-4.05%) ⬇️
events/cloudwatch_logs.go 60.00% <0.00%> (-3.64%) ⬇️
lambda/runtime_api_client.go 75.00% <0.00%> (-2.78%) ⬇️
events/attributevalue.go 80.32% <0.00%> (-2.44%) ⬇️
cmd/build-lambda-zip/main.go 33.82% <0.00%> (-2.29%) ⬇️
lambda/panic.go 87.87% <0.00%> (-2.13%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55dc88b...4ce5148. Read the comment docs.

@bmoffatt
Copy link
Collaborator

can you include a test case that exercises this?

@shogo82148 shogo82148 changed the title safeMarshal should escape the error message rename safeMarshal to mustMarshal Sep 23, 2020
payload, err := json.Marshal(v)
if err != nil {
return []byte(fmt.Sprintf(serializationErrorFormat, err.Error()))
panic("lambda: " + err.Error())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I've misunderstood.
json.Marshal(v) doesn't return any error, because we know *messages.InvokeResponse_Error can be marshaled.
So, we don't need to treat this case, and just enough to panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmoffatt
Copy link
Collaborator

I don't think panic is the right thing to do here

  • "Runtime.SerializationError" is no longer reported to the data-plane, so the client of the function doesn't know what went wrong
  • panicing also causes the next invoke to be a a kind of cold start, which doesn't seem appropriate if the only problem was a response serialization error

@bmoffatt bmoffatt closed this Sep 24, 2020
@shogo82148
Copy link
Contributor Author

I tried to write an AWS Lambda handler that causes "Runtime.SerializationError" for testing, but I couldn’t.
There is no way to cause "Runtime.SerializationError", so calling panic is simplest way to handle the error.

the safeMarshal function might return invalid JSON if err.Error() contains special characters of JSON.

This problem remains unresolved.
I'll try another fix.

@shogo82148 shogo82148 deleted the fix-serialize-error branch September 24, 2020 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants