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

make Curl_get_line not require a newline on the last line #9973

Closed
wants to merge 2 commits into from

Conversation

robdewit
Copy link
Contributor

Improve backwards compatibility by allowing a last line not ending with newline char.

@robdewit
Copy link
Contributor Author

robdewit commented Nov 23, 2022

Lines 51-60 could be left out if calling code is not relying on the newline as terminator.

@robdewit robdewit marked this pull request as ready for review November 23, 2022 16:22
lib/curl_get_line.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Nov 24, 2022

I suppose the title means improved backwards compatibility for older .netrc handling? This function is used for more than that though.

@robdewit
Copy link
Contributor Author

I already thought it would be used in more places. For netrc only, the insertion of the missing newline would not be necessary. I guess it would be good practice for any file being read, to not throw away the last line if there is no newline. Unless, of course, the newline is part of the config specs.

@bagder bagder changed the title Improve backwards compatibility make Curl_get_line not require a newline on the last line Nov 24, 2022
@robdewit robdewit requested a review from bagder November 25, 2022 11:14
lib/curl_get_line.c Show resolved Hide resolved
lib/curl_get_line.c Outdated Show resolved Hide resolved
bagder
bagder approved these changes Nov 29, 2022
@bagder
Copy link
Member

bagder commented Nov 29, 2022

The many Windows test 8 failures need some closer inspection though...

lib/curl_get_line.c Outdated Show resolved Hide resolved
lib/curl_get_line.c Show resolved Hide resolved
@bagder

This comment was marked as outdated.

}
return 0;
}
UNITTEST_STOP
Copy link
Member

Choose a reason for hiding this comment

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

I love the unit test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any Windows environment at hand, but I'm confused. eof() should only trigger on the condition of having reached the end of input, not some control character. So my guess would be that the curl_get_line() function should just return the string including whatever control character is in there. It is then up to the parser that calls curl_get_line to decide what to do with that.

I added a line containing ^Z in the unit test just to check what Windows would make of it.

Maybe the code needs something like #ifndef WIN32 to not break compatibility in WIndows - which was the whole purpose of the PR anyway - maximum downwards compatibility.

Copy link
Contributor Author

@robdewit robdewit Dec 1, 2022

Choose a reason for hiding this comment

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

According to the OK result of the changed unit test on Windows, curl_get_line() seems to correctly return a string including the ^Z character. So I guess the fail in test 8 is caused by differences in parsing the cookie on Windows compared to Linux or MacOS.

@bagder What to do? Remove the offending line from test 8 won't fix the difference in test output, but it looks like I can't fix the failing of test 8 in curl_get_line().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the difference in processing is caused by cookie.c:1267 opening the file as FOPEN_READTEXT, which makes Window interpret (some) control characters. The previous version of curl_get_line() would discard the line as a partial read because of the missing \n. The new version reads the data up until, but excluding the ^Z, so the cookie.c:452 invalid_octets() function does not throw the data away. What's left is a valid cookie26= cookie.

Still no clue how to handle this, I guess there's good reasons to use the FOPEN_READTEXT.

Copy link
Member

Choose a reason for hiding this comment

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

As I recall things we went with text file simply because cookies are text. there were no considerations made for silly ^Z-handling. I just submitted #10017 to change this, let's see how that goes 😄

bagder added a commit that referenced this pull request Dec 2, 2022
On Windows there is a difference and for text files, ^Z means end of
file which is not desirable.

Ref: #9973
bagder added a commit that referenced this pull request Dec 2, 2022
On Windows there is a difference and for text files, ^Z means end of
file which is not desirable.

Ref: #9973
Closes #10017
@bagder
Copy link
Member

bagder commented Dec 2, 2022

if you rebase this PR onto master now and force-push, I think the ^Z issue on Windows should be gone and I think we are ready to merge

bagder
bagder approved these changes Dec 2, 2022
@bagder
Copy link
Member

bagder commented Dec 2, 2022

Thanks a lot for your hard work and patience with this!

@fredericgermain
Copy link

I guess it more that you should not merge a commit and do a release on the same day? (linked to #9789 where you close comments).

@bagder
Copy link
Member

bagder commented Jan 30, 2023

  1. Why not?
  2. How is that relevant to this bugfix?

@bagder
Copy link
Member

bagder commented Jan 30, 2023

If you want to help out in the project, please do. We will appreciate it. Coming here months later just because Apple is slow to ship the latest curl version is not helpful.

@fredericgermain
Copy link

I think integrating a commit and doing a release the same day with it is a very bad practice., but it's your project in the end.

This bugfix helped fix the problem in release 7.86.0, which you pushed in #9789, exactly the day of the tagging of 7.86.0. And you closed the comment about discussing the issue, so adding here.

I'm not an Apple fan boy, but I'm very skeptical about your comment "That sounds like an Apple problem". Most distribution makes months to integrate new releases, but you might have changed to have a faster integration when it's linked to security fixing, and it's not the case here.
I'd be actually interested to know the distribution you're using, we'll see if they are actually faster than Apple to ship it.

Please understand that curl is used by many people, and it might need nice to have a longer insight into the commits being integrated.

I would have helped with the project, but it's fixed by this commit, so I don't have anything to add so far.

Also, I'd like to add that closing a ticket for comments is quite childish.

@bagder
Copy link
Member

bagder commented Jan 30, 2023

I closed that issue because your complaining did not add anything of value, and virtually nobody is going to see or follow a thread of comments added to a long-time closed issue. You mentioned that Apple shipped a curl with a bug we fixed in a later release.

Now I'm going to waste even more time and effort on this non-issue because you insist. Because I am childish I guess.

If you want to argue about or contribute to improve our development process then you are most welcome to do so, but doing this in an old closed issue or pull request is also a fairly bad way at doing it because it will not reach the audience that would be interested. You should rather take that in a new discussion here on GitHub or even better on the curl-library mailing list. I would then of course urge you to rather bring your proposals for what to improve rather than to just whine on something that failed previously.

It is now "an Apple problem" because Apple builds and ships curl bundled with macOS and they could opt to ship 7.87.0 already now. We fixed the problem already. Numerous distributions and operating systems already provide the latest curl release to their users. I personally use Debian unstable, it usually offers the latest curl release within days of our releases.

To the issue of merging a bugfix just days before a release:

  • this particular bugfix was a security fix, which we always merge late in the cycle on purpose because it will reveal to observers that there was a security flaw that can be exploited. Still, it was offered as a pull request, it ran through all the tests fine and not a single person objected to it or remarked on its functionality - until several weeks later. Thus, the closeness in time to release was actually irrelevant.
  • a fixed bug is something we want to include in the next release and most users would expect us to ship bugfixes that we have already working, in the pending release. It's a balance of course, but just giving something more time is also not a guarantee for us finding problems with code.
  • I think our development process should rely on procedures and established ways that do not make us have to just wait for some period of time between a fix and a release. Code style, reviews, tests, verification, code scans, tortures should be enough. In this particular case, what failed us was that we did not have a test case using this edge case.

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.

None yet

3 participants