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

Bugfix: Fix segfault when -H @headerfile is empty #2797

Closed
wants to merge 1 commit into from

Conversation

sm0svx
Copy link
Contributor

@sm0svx sm0svx commented Jul 26, 2018

This commit will fix a bug where the curl binary would crash if the -H
command line option was given a filename to read using the @filename
syntax but that file was empty.

This commit will fix a bug where the curl binary would crash if the -H
command line option was given a filename to read using the @filename
syntax but that file was empty.
@bagder
Copy link
Member

bagder commented Jul 27, 2018

Lovely. Any chance you could add a test case for this as well?

@jay
Copy link
Member

jay commented Jul 27, 2018

This looks like it was a simple oversight I don't think it needs a test case. Also I think a better fix for this would be return an empty buffer if the file is empty.

@sm0svx
Copy link
Contributor Author

sm0svx commented Jul 27, 2018

@jay: I felt it was a lot safer to change the error handling locally in that spot only since the file2memory function is used in a number of other places. The risk of introducing new bugs is much bigger if the behaviour of the file2memory function is changed, don't you agree?

@bagder: I find it a bit hard to understand how the tests are supposed to work. I used cmake to build curl but a "make test" does not seem to be the right way to trigger the tests. I get:

Running tests...
Test project /tmp/curl/build
No tests were found!!!

Reading the README in the tests directory I get the impression that autotools should be used instead of cmake. Is autotools used when building tests and cmake when building the lib and binaries? I'm a bit confused. Can you point me in the right direction, or should we skip writing a test for this as @jay suggests?

@sm0svx
Copy link
Contributor Author

sm0svx commented Jul 27, 2018

I now saw the note about the cmake buildsystem beeing poorly maintained. I'll try building it all using autotools instead. I guess that is the preferred way to build curl.

@bagder
Copy link
Member

bagder commented Jul 28, 2018

I'll add a test case when I merge this!

@bagder bagder closed this in 3e9b3a3 Jul 28, 2018
bagder added a commit that referenced this pull request Jul 28, 2018
@bagder
Copy link
Member

bagder commented Jul 28, 2018

Thanks!

xquery pushed a commit to xquery/curl that referenced this pull request Aug 9, 2018
The curl binary would crash if the -H command line option was given a
filename to read using the @filename syntax but that file was empty.

Closes curl#2797
xquery pushed a commit to xquery/curl that referenced this pull request Aug 9, 2018
falconindy pushed a commit to falconindy/curl that referenced this pull request Sep 10, 2018
The curl binary would crash if the -H command line option was given a
filename to read using the @filename syntax but that file was empty.

Closes curl#2797
falconindy pushed a commit to falconindy/curl that referenced this pull request Sep 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants