Skip to content

curl_get_line: handle lines ending on the buffer boundary #17036

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

Closed
wants to merge 1 commit into from

Conversation

cole-h
Copy link
Contributor

@cole-h cole-h commented Apr 12, 2025

Very similar to 9f8bdd0, but affects e.g. netrc file parsing.

Suggested-by: Graham Christensen graham@grahamc.com


I opted to write a Perl test for this, since I didn't know how dynamic the %HOSTIP is, and triggering this bug requires the netrc line to be exactly 127 characters (so if it changes away from 127.0.0.1 in the future, this test would start to fail).

I am very far from an experienced Perl author, but I didn't see any examples of command type="shell" in the repo, so I just adapted an existing Perl test runner thing.

@cole-h
Copy link
Contributor Author

cole-h commented Apr 12, 2025

Running make test on the box where I developed this patch gave me:

TESTDONE: 1800 tests were considered during 410 seconds.
TESTDONE: 1615 tests out of 1615 reported OK: 100%

Not sure why it's failing in CI, but I'd be glad to fix that up if you can provide any tips on how to do so!

@testclutch
Copy link

Analysis of PR #17036 at 6b3c929f:

Test 744 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 2033 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@dfandrich
Copy link
Contributor

It doesn't look to me like you need a helper perl script to test this. Write the netrc file with a /testcase/client/file section (I'm giving XPath style there) like in test130. You can use %repeat[100 x a]% to pad the file to an exact length, although it would need to be precalculated (see test2080 for an example). Use a /testcase/client/precheck section like test1082 to ensure the address is what you expect. Then use a regular /testcase/client/command section similar to test130 again to actually run the curl command.

@bagder
Copy link
Member

bagder commented Apr 12, 2025

If you use a proxy for the test, you can use a netrc file with a fixed size since the hostname is not going to change.

The netrc file can be provided with a 127 byte line like done in the new test 743 added in 9f8bdd0

Very similar to 9f8bdd0, but affects
e.g. netrc file parsing.

Suggested-by: Graham Christensen <graham@grahamc.com>
@cole-h cole-h force-pushed the cole-h/push-rvnyqyunrswv branch from 6b3c929 to 19810a1 Compare April 12, 2025 16:13
@cole-h
Copy link
Contributor Author

cole-h commented Apr 12, 2025

Thanks for the suggestions, both!

I went with @bagder's suggestion in the end, and it is indeed much nicer than making a Perl helper 😆

TESTDONE: 1800 tests were considered during 544 seconds.
TESTDONE: 1561 tests out of 1561 reported OK: 100%

@bagder bagder closed this in 2404a11 Apr 15, 2025
@bagder
Copy link
Member

bagder commented Apr 15, 2025

Thanks!

@cole-h cole-h deleted the cole-h/push-rvnyqyunrswv branch April 15, 2025 15:52
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
Very similar to 9f8bdd0, but affects
e.g. netrc file parsing.

Suggested-by: Graham Christensen <graham@grahamc.com>

Add test 744 to verify

Closes curl#17036
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
Very similar to 9f8bdd0, but affects
e.g. netrc file parsing.

Suggested-by: Graham Christensen <graham@grahamc.com>

Add test 744 to verify

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

Successfully merging this pull request may close these issues.

4 participants