Check if res is nil before copy Header

openfaas / nats-queue-worker

Queue-worker for OpenFaaS with NATS Streaming

https://docs.openfaas.com/reference/async/

MIT License

128 stars 59 forks source link

Check if res is nil before copy Header #26

Closed lybroman closed 4 years ago

lybroman commented 6 years ago

If res is nil with err, the container would be killed by SIGSEGV. Runtime error: invalid memory address or nil pointer dereference

Description

Check the src is not nil before copying Header

Motivation and Context

When adding a new feature according to our need, I found this minor issue.

How Has This Been Tested?

I tested this code by hardcode the functionRes to nil when calling postResult.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I've read the CONTRIBUTION guide
[x] I have signed-off my commits with git commit -s
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

derek[bot] commented 6 years ago

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.

alexellis commented 6 years ago

Thank you for raising this. We use that method in other places, so maybe it should do some error checking to see if the value is nil before dereferencing.

Alex

alexellis commented 6 years ago

Hi @lucasroesler please could you identify usages across the projects and rose an issue on the FaaS repo? Also happy for @lybromam to do this.

Alex

LucasRoesler commented 6 years ago

There are the locations I have found so far, I will work on getting issues written and hopefully some failing unit tests

https://github.com/openfaas/faas-netes/blob/d2a9878e8bf31f12290389f6a7bdc913f8ca0718/handlers/proxy.go#L60-L64 https://github.com/openfaas/faas-swarm/blob/c890cba302d059de8edbef3f3de7fe15444b1ecf/handlers/proxy.go#L137-L141 https://github.com/openfaas/nats-queue-worker/blob/15287c9b2af293cee0c545ccc014a68e1f0a4424/main.go#L247-L251 https://github.com/openfaas-incubator/openfaas-operator/blob/db30249630e2d7aedc75517e847088c2a1fc614e/pkg/server/proxy.go#L52-L55 https://github.com/openfaas/faas/blob/e655f28e8d8532bd2e07aa699e9a4551a40adc67/gateway/handlers/forwarding_proxy.go#L77-L91

alexellis commented 6 years ago

Thank you for leading this. I think there will be several instances in the incubator repo too.

alexellis commented 4 years ago

Looks good, however it seems that we have this change already so I'll close. If that's incorrect, feel free to send a new rebased PR. Thanks for contributing.