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

Jump to bottom

connect: On linux, enable reporting of all ICMP errors on UDP sockets #6341

Closed
wants to merge 1 commit into from
Closed

connect: On linux, enable reporting of all ICMP errors on UDP sockets #6341

wants to merge 1 commit into from

Conversation

crrodriguez
Copy link
Contributor

The linux kernel does not report all ICMP errors back to userspace
due to historical reasons.
IP*_RECVERR sockopt must be turned on to have the correct behaviour
which is to pass all ICMP errors to userspace.

See
https://bugzilla.kernel.org/show_bug.cgi?id=202355<

PS: You almost certainly want this option to be set in your DNS resolver too (ares, or whatever, glibc was already fixed)

@bagder
Copy link
Member

bagder commented Dec 17, 2020

Are there any particular situations when this helps curl users? It seems like a good thing to me but I'm also curious since I can't recall anyone ever missing these errors in the past...

@crrodriguez
Copy link
Contributor Author

Are there any particular situations when this helps curl users? It seems like a good thing to me but I'm also curious since I can't recall anyone ever missing these errors in the past...

When there are network errors, yes, it will help, applications will fail immediately rather than hang or wait for timeout. You have to simulate flaky network like for example what is described on the equivalent glibc bug report https://sourceware.org/bugzilla/show_bug.cgi?id=24047

@bagder
Copy link
Member

bagder commented Dec 17, 2020

Just some checksrc complaints:

./connect.c:1580:11: warning:  switch with space (SPACEBEFOREPAREN)
     switch (addr->family) {
           ^
./connect.c:1582:62: warning:  sizeof with space (SPACEBEFOREPAREN)
         setsockopt (*sockfd, SOL_IP, IP_RECVERR, &one, sizeof (one));
                                                              ^
./connect.c:1585:66: warning:  sizeof with space (SPACEBEFOREPAREN)
         setsockopt (*sockfd, SOL_IPV6, IPV6_RECVERR, &one, sizeof (one));
                                                                  ^

@crrodriguez
Copy link
Contributor Author

Just some checksrc complaints:

./connect.c:1580:11: warning:  switch with space (SPACEBEFOREPAREN)
     switch (addr->family) {
           ^
./connect.c:1582:62: warning:  sizeof with space (SPACEBEFOREPAREN)
         setsockopt (*sockfd, SOL_IP, IP_RECVERR, &one, sizeof (one));
                                                              ^
./connect.c:1585:66: warning:  sizeof with space (SPACEBEFOREPAREN)
         setsockopt (*sockfd, SOL_IPV6, IPV6_RECVERR, &one, sizeof (one));
                                                                  ^

Huh.. yeah, would ned to fix that..

@crrodriguez
Copy link
Contributor Author

That should pass..

@bagder
Copy link
Member

bagder commented Dec 19, 2020

It fails on macOS:

connect.c:1518:7: error: unused variable 'one' [-Werror,-Wunused-variable]
  int one = 1;
      ^
1 error generated.

@crrodriguez
Copy link
Contributor Author

It fails on macOS:

connect.c:1518:7: error: unused variable 'one' [-Werror,-Wunused-variable]
  int one = 1;
      ^
1 error generated.

..and I can't declare it within the #if because of strict iso C..ok.. will fix that too.

@bagder
Copy link
Member

bagder commented Dec 21, 2020

You can move the declaration to be within the if block, right?

@crrodriguez
Copy link
Contributor Author

You can move the declaration to be within the if block, right?

Yes, but then in other target there will be a complain that "ISO C90 forbids mixed declarations and code" and will fail too 💥

@bagder
Copy link
Member

bagder commented Dec 21, 2020 •
edited

Why? C89/C90 says variable declarations can be in the beginning of any block (that starts with a { brace).

@crrodriguez
connect: On linux, enable reporting of all ICMP errors on UDP sockets
9123ee4 
The linux kernel does not report all ICMP errors back to userspace
due to historical reasons.
IP*_RECVERR sockopt must be turned on to have the correct behaviour
which is to pass all ICMP errors to userspace.

See
https://bugzilla.kernel.org/show_bug.cgi?id=202355
@crrodriguez
Copy link
Contributor Author

Why? C89/C90 says variable declarations can be in the beginning of any block (that starts with a { brace).

Yeah, I ammended the commit now.

@bagder
Copy link
Member

bagder commented Dec 21, 2020

Thanks!

@bagder bagder closed this in d13179d Dec 21, 2020
@crrodriguez crrodriguez deleted the linux_report_icmp_errors branch December 22, 2020 14:36
@bagder bagder mentioned this pull request Jul 26, 2022
Closed
bagder added a commit that referenced this pull request Jul 26, 2022
@bagder
connect: revert the use of IP*_RECVERR
cf93570 
The options were added in #6341 and d13179d, but cause problems: Lots of
POLLIN event occurs but recvfrom read nothing.

Reported-by: Tatsuhiro Tsujikawa
Fixes #9209
bagder added a commit that referenced this pull request Jul 26, 2022
@bagder
connect: revert the use of IP*_RECVERR
39fe96d 
The options were added in #6341 and d13179d, but cause problems: Lots of
POLLIN event occurs but recvfrom read nothing.

Reported-by: Tatsuhiro Tsujikawa
Fixes #9209
@bagder bagder mentioned this pull request Jul 26, 2022
Closed
bagder added a commit that referenced this pull request Jul 27, 2022
@bagder
connect: revert the use of IP*_RECVERR
3141062 
The options were added in #6341 and d13179d, but cause problems: Lots of
POLLIN event occurs but recvfrom read nothing.

Reported-by: Tatsuhiro Tsujikawa
Fixes #9209
Closes #9215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
connecting & proxies
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@crrodriguez @bagder