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

tool: change some fopen failures from warnings to errors #11677

Closed
wants to merge 2 commits into from

Conversation

jay
Copy link
Member

@jay jay commented Aug 15, 2023

  • Error if a user-specified file cannot be opened.

  • Change file2memory and file2string functions to return an error if passed a null FILE, instead of returning zero length data.

Prior to this change, if a user of the curl tool specified a file that could not be opened for reading data then curl would show a warning instead of an error and treat it as zero length data. For example, a POST using --data @filenametypo would cause a zero length POST which is probably not what the user intended.

Closes #xxxx

src/tool_getparam.c Dismissed Show dismissed Hide dismissed
@jay
Copy link
Member Author

jay commented Aug 16, 2023

I haven't looked at all the CI results but I can see there are at least two tests failing 268 344 both of which expect curl to be able to handle non existent files. I can remove those tests but I think it's pretty clear now it was someone's intention to allow this behavior.

For example rather than create foo.txt and bar.txt beforehand if the open fails just treat it as empty

--etag-compare foo.txt --etag-save foo.txt

--cookie bar.txt --cookie-jar bar.txt

hm

@jay
Copy link
Member Author

jay commented Aug 18, 2023

I plan to dial this PR back somewhat since it seems there are some intended cases where it may be useful to allow non-existent files to be treated as empty.

- Error on missing input file for --data, --data-binary,
  --data-urlencode, --header, --variable, --write-out.

Prior to this change if a user of the curl tool specified an input file
for one of the above options and that file could not be opened then it
would be treated as zero length data instead of an error. For example, a
POST using `--data @filenametypo` would cause a zero length POST which
is probably not what the user intended.

Closes #xxxx
@jay
Copy link
Member Author

jay commented Aug 21, 2023

Ok I limited this change to error on missing input files for --data, --data-binary, --data-urlencode, --header, --variable, --write-out. test268 is going to fail because it tests to make sure --data will POST zero length content when passed a non-existent filename. IMO if the user passes a non-existent file for POST that's an accident not intentional. From 2005 639857c @bagder

@bagder
Copy link
Member

bagder commented Aug 21, 2023

I think trying to post a file that can't be read should rather cause an error.

@github-actions github-actions bot added the tests label Aug 21, 2023
test 268 is a check to make sure a zero-length POST is sent when passed
a non-existent file but we now error in this case.
@jay jay closed this in aacbeae Aug 30, 2023
@jay jay deleted the tool_fail_hard branch August 30, 2023 07:24
@icing
Copy link
Contributor

icing commented Aug 30, 2023

It does not make work easier when we merge changes that break CI. Just saying.

@jay
Copy link
Member Author

jay commented Aug 30, 2023

It does not make work easier when we merge changes that break CI.

Yes. This occasionally happens with all of us due to CI fatigue. We get the occasional random break due to CI arbitrary weirdness pretty much all the time and it makes the actual failures less likely to be noticed. I don't wait for 100% green check marks, instead I go through the CI and try to determine if my change is responsible (and oops, it was and I missed it). A lot of work has been done to improve the situation but it may happen occasionally.

ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
- Error on missing input file for --data, --data-binary,
  --data-urlencode, --header, --variable, --write-out.

Prior to this change if a user of the curl tool specified an input file
for one of the above options and that file could not be opened then it
would be treated as zero length data instead of an error. For example, a
POST using `--data @filenametypo` would cause a zero length POST which
is probably not what the user intended.

Closes curl#11677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants