Skip to content

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

Closed
crrodriguez wants to merge 1 commit into
curl:masterfrom
crrodriguez:linux_report_icmp_errors
Closed

connect: On linux, enable reporting of all ICMP errors on UDP sockets#6341
crrodriguez wants to merge 1 commit into
curl:masterfrom
crrodriguez:linux_report_icmp_errors

Conversation

@crrodriguez

Copy link
Copy Markdown
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

bagder commented Dec 17, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
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

bagder commented Dec 17, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
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 crrodriguez force-pushed the linux_report_icmp_errors branch from 60c7a49 to 261e408 Compare December 17, 2020 17:00
@crrodriguez

Copy link
Copy Markdown
Contributor Author

That should pass..

@bagder

bagder commented Dec 19, 2020

Copy link
Copy Markdown
Member

It fails on macOS:

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

@crrodriguez

Copy link
Copy Markdown
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

bagder commented Dec 21, 2020

Copy link
Copy Markdown
Member

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

@crrodriguez

Copy link
Copy Markdown
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

bagder commented Dec 21, 2020

Copy link
Copy Markdown
Member

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

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 crrodriguez force-pushed the linux_report_icmp_errors branch from 261e408 to 9123ee4 Compare December 21, 2020 12:10
@crrodriguez

Copy link
Copy Markdown
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

bagder commented Dec 21, 2020

Copy link
Copy Markdown
Member

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 added a commit that referenced this pull request Jul 26, 2022
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 27, 2022
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

Development

Successfully merging this pull request may close these issues.

2 participants