Skip to content
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

Fix unchecked calloc bug in network.c #492

Closed
wants to merge 1 commit into from

Conversation

Robotic-Brain
Copy link
Contributor

Hi, i just read this Blog-post by @fluca1978 and stumbled across another bug in the same function. See below.

Also I'm curious as to why sport is heap-allocated in the first place? Wouldn't it be more efficient (and remove some error checking) if it was stack-allocated? In that case i would like to update the patch to convert sport into a stack-allocated, fixed size string.

Problem summary

At line 636 sport will always be non-NULL because it has already been checked in line 608 using early return on error. Meanwhile result has never been checked against NULL and is subsequently used in line 696. Probable cause of the bug is inadequate copy-and-pasting of code. I assume this changes it to the intended behavior.

Disclaimer

I did not try to compile or even checkout the code locally. This bug was found through static-analysis/code-review only.
Please verify that it compiles and passes any automatic tests before merging!

@jesperpedersen
Copy link
Collaborator

Please, add yourself to

AUTHORS
doc/manual/97-acknowledgement.md
doc/manual/advanced/97-acknowledgement.md

then squash and force push

At line 636 sport will always be non-NULL because it has already been checked in line 608 using early return on error.
Meanwhile result has never been checked against NULL and is subsequently used in line 696.
Probable cause of the bug is inadequate copy-and-pasting of code. I assume this changes it to the intended behavior.
@Robotic-Brain
Copy link
Contributor Author

The force push messed up the PR. new PR is #493

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants