Fix #39: getaddrinfo numeric error codes are now translated #40

LeeTibbert · 2022-09-10T15:03:22Z

Numeric error codes returned from getaddrinfo are now translated to text.
Fixes #39.

Previously:

==> X epollcat.TcpSuite.LeeT gai error msg 0.01s
java.io.IOException: getaddrinfo: -9

After this PR:

==> X epollcat.TcpSuite.LeeT gai error msg 0.01s
java.io.IOException: getaddrinfo:
  Address family for hostname not supported address: 240.0.0.1  port: 80

Most developers will never see these messages, but those who do will
appreciate the time savings.

LeeTibbert · 2022-09-10T15:12:12Z

The (relatively) new Suite 20 second timeout has already saved me
a lot of time today, thank you.

LeeTibbert · 2022-09-10T15:15:07Z

The context for this change is that as I probe IPv6 support, I cause lots of gaiaddrinfo errors, sometimes
intentionally and sometimes not. I have always slacked off on gai errors and printed the code. A few
years of "I have been meaning to fix that!" translated into action. Especially as I face the prospect
of my introducing more gai errors.

LeeTibbert · 2022-09-10T15:16:47Z

A CI that does not take 2 hours to run (and then fail). What a joy!

armanbilge

Thanks for the PR! Happy to see error messages being improved.

Do you think we can simulate some of these errors in our test suite? If so, then what would you think about aiming to make our error messages consistent with those used by the JVM/JDK?

armanbilge · 2022-09-10T15:15:54Z

core/src/main/scala/epollcat/internal/ch/SocketHelpers.scala

+
+    val port = addr.getPort.toString
+
+    s"${gaiMsg} address: ${adr}  port: ${port}"


What are your thoughts on the security concersn with including the address and port in the error message? We recently had this exact discussion on the Typelevel Discord.

Some insights from Michael Pilquist:

any security concerns about including host/port in the error message? e.g., imagine an http4s route that tries to make a request to some other service and fails with timeout -- that exception message would get propagated to client, exposing internal hosts/port info

Thank you for the review & raising the concern.

TL;DR
If there is any chance that the info will impede upstream adoption, I will remove
it and use a private version. No problem.

Longer discussion:

Personal opinion:

Bad actors are incredibly clever at exploiting any lack of entropy. Witness all the
Intel & AMD speculative execution hacks.

People trying to fix problems depend upon there being a lack of entropy: i.e. information.

The very fact that a getaddrinfo message of any kind is being reported gives
suitably motivated bad actors information. Removing even a simple message
makes life difficult/impossible for the "fixers".

"fixers" can be overwhelmed by too much information, so finding the appropriate
amount is a changing goal.

Here the port number probably gives away the most information.
The address is in n.n.n.n or (soon) xxxx::x notation, not a string
"secrets.com". The numeric address can be translated, but
it takes someone's time and a bit of skill.

The understanding of "Industry best-practice" changes over time.
At this point and for the foreseeable future I think giving at
least the basic getaddrinfo code translation can be defended
as "Industry standard".

I think we concur that Rapid prototyping is not an occasion for a security give-away.
Security analysis is essential but needs to be considered in scope: valid security, extra-cautious security,
paranoid security, and security-theater.

I think the base PR is safe but not worth investing the time to defend to the
"deny any information at all" folks.

Thanks for sharing your thoughts. Unfortunately I am not a security expert, so in doubt I defer to the JVM: can we simulate some of these errors in our test suite, and make the exceptions match what the JVM does in the same situation.

Looks like Java 17 ServerSocket bind distinguishes EAI_FAMILY (unsupported address family)
and throws an IllegalArgumentException. In other cases it throws IOException. The
current epoll code always throws IOException (with a message that would give the unsupported
AF, but probably not tell which af it got). That is fixable.

I can try to provoke the a SocketServer bind IOException and see exactly what
text it returns. Will take time (day or two-ish).

Then I have to check the java.nio Channel area to see if there is any
complication/change in message.

I can submit the simple (no address, no port) gai_message updates
so they do not get lost on my end. Understanding that they are on
hold whilst I find out what Java does.

Thanks for understanding! That sounds like a good plan to me.

Update:

Java 17 AsynchronousSeverSocketChannel throws UnsupportedAddressTypeException not IllegalArgumentException
for wrong AF. I expect AsynchronousSocketChannel to to the same but will check before implementing.

core/src/main/scala/epollcat/internal/ch/SocketHelpers.scala

LeeTibbert · 2022-09-11T00:27:44Z

Arman,

Per our helpful discussion,:

I altered two getaddrinfo error paths to return the same kind of exception on both JVM & Native.
That exception has no arguments, so no issues with host names. UnsupportedAddressTypeException
is a RuntimeException while IOException is a java.lang.Exception.

This particular path should never be seen, but if it is, they now match

JVM describes no getaddrinfo messages. If any of those show up in
epollcat native, we will be glad to have them to give us a clue. They
are 'should never happen'.

I edited AsynchronousServerSocket#bind so that now both JVM & Native
throw the same EADDRNOTAVAIL exception.

I had to fudge things a bit for 0.4.5 (?) because EADDRNOTAVAIL is not available there.
It should be there if the backport merge from 0.5.0 happened correctly. It is in 0.5.0.
If you have a better solution, let me know. Else avert your eyes for that short section.

I checked & tickled AsynchronousSocket#connect to see if I could get the Native
and JVM Exception to align in at least the common case. Limited joy.
When presented with a known bogus IPv4 address, "10%en0", they both
give the same message. Good so far.

Where things differ is that JVM rightly detects that InetAddress.getByName("240.0.0.") is
bogus and throws an Exception. SN does not detect the bogus address and attempt
to connect to something (I ran out of time & drive before I could ascertain what), and
eventually times out.

I think the proper path forward is for me to create a Scala Native Issue. I had been
hoping to do some simplification in that now 12+ years old code. Harmony did
not have the advantage of inet_pton or inet_ntop.

This is a second evolution, there are probably 99 or more evolutions to go, but
not tonight.

PS: During the course of this work, I discovered that strerror & kin are totally
missing from Scala Native. They are used to convert errno.errno numerics
to text strings. I created a Scala Native Issue. epollcat cuts another notch
on the stick used to record SN bugs found & reported.

armanbilge · 2022-09-11T14:47:36Z

Thanks, Lee, that's looking great!

For every new exception that you add, would you mind adding a test as well? I already have a few existing tests to verify that exceptions match.

epollcat/tests/shared/src/test/scala/epollcat/TcpSuite.scala

Lines 232 to 261 in a4544f0

    
           test("ConnectException") { 
        
             IOServerSocketChannel 
        
               .open 
        
               .evalTap(_.bind(new InetSocketAddress(0))) 
        
               .use(_.localAddress) 
        
               .flatMap(addr => IOSocketChannel.open.use(_.connect(addr))) 
        
               .interceptMessage[ConnectException]("Connection refused") 
        
           } 
        
           test("BindException") { 
        
             IOServerSocketChannel 
        
               .open 
        
               .evalTap(_.bind(new InetSocketAddress(0))) 
        
               .evalMap(_.localAddress) 
        
               .use(addr => IOServerSocketChannel.open.use(_.bind(addr))) 
        
               .interceptMessage[BindException]("Address already in use") 
        
           } 
        
           test("ClosedChannelException") { 
        
             IOServerSocketChannel 
        
               .open 
        
               .evalTap(_.bind(new InetSocketAddress(0))) 
        
               .evalMap(_.localAddress) 
        
               .use { addr => 
        
                 IOSocketChannel.open.use { ch => 
        
                   ch.connect(addr) *> ch.shutdownOutput *> ch.write(ByteBuffer.wrap(Array(1, 2, 3))) 
        
                 } 
        
               } 
        
               .intercept[ClosedChannelException] 
        
           }

LeeTibbert · 2022-09-11T20:00:24Z

Agreed, a good concept.

When you say "new" do you mean new as in adding new "FooException.scala"
or new as in adding a new use of an existing Exception?

java.nio.channels.UnsupportedAddressTypeException

In this particular case, I had that concern in mind, as it
is my usual practice, albeit usually in an "AdvancedSuite"
and not a daily driver Suite.

I could not figure out a way to cause the Exception at runtime.
I had to hand edit local copies of each EpollAsynchronousSocketChannel and
EpollAsynchronousServerSocketChannel to inject the error.
If I missed something or if you can thing of a way, I can write a suitable test for each.
I will keep the consideration on the back of my brain.

EADDRNOTAVAIL

I will add a runtime test for this. I have a number of cases which
evoke it in my private development area.

armanbilge · 2022-09-11T20:06:49Z

When you say "new" do you mean new as in adding new "FooException.scala"
or new as in adding a new use of an existing Exception?

Yes, every new use. Essentially every time we add a new conditional branch to our sources.

I could not figure out a way to cause the Exception at runtime.

Yeah, no problem. If we can find a way it's always good to add a test to lock in the behavior, if it's non-trivial then let's just move on.

Thanks!

LeeTibbert · 2022-09-11T20:43:43Z

I added a test for BindException EADDRNOTAVAIL. It can probably be minimized and/or
tightened up by someone with better cats skills than I. I believe that what is there is correct, albeit
probably not idiomatic.

The current test focuses on the failing call bind. The models I had to work from
all did distracting things after the bind before they intercepted the Exception.

LeeTibbert · 2022-09-11T20:45:02Z

Yes, every new use. Essentially every time we add a new conditional branch to our sources.
Understood.

if it's non-trivial then let's just move on.
Agreed.

LeeTibbert · 2022-09-11T20:50:00Z

And, of course, macOS has a slightly different message "'Can't assign requested address" (macOS).
Linux uses "Cannot assign requested address"

A learning opportunity for me to figure out how to get interceptMessage to handle "or" or
regex.

armanbilge · 2022-09-11T20:54:57Z

Ha, annoying! Instead of writing a regex why not use an if-else switch with the OS check?

LeeTibbert · 2022-09-11T21:02:06Z

Ha, annoying! Instead of writing a regex why not use an if-else switch with the OS check?

I am studying munit-cats-effect in another window. Looks like .interceptMessage does not
provide alternatives. I need to figure out how to either set an expectedText variable before
or how to do a ".if" with the Resource (?) currently being piped to .interceptMessage.
I guess that getting the back of the gargoyle's nose hairs carved correctly is a sign
of craftsmanship.

I am off to look for models.

armanbilge · 2022-09-11T21:03:37Z

tests/shared/src/test/scala/epollcat/TcpSuite.scala

+          _ <- ch.bind(new InetSocketAddress("240.0.0.1", 0))
+        } yield ()
+      }
+      .interceptMessage[BindException]("Cannot assign requested address")


@LeeTibbert why not do this?

Suggested change

.interceptMessage[BindException]("Cannot assign requested address")

.interceptMessage[BindException](

if (isLinux) "Cannot assign requested address"

else "Can't assign requested address"

)

Thank you for the suggestion. I did not know that was possible. Learning new things is good,
but also an exercise in humility.

The code snippet works fine on SN, after importing LinktimeInfo.

Understandably, it fails on JVM. May need to factor this test out and
use the fact that epollcat is a multi-project to have a new Suite
of OS specific tests. I hate having duplicate code, as it
is hard to keep in sync. So, worst case, I/we can fix this.

I am off to look around to see how the test might determine
the OS it is running on. (perhaps more complicate code
within the intercept message to read the System Property
"os.name") Can the code (thunk?) have more than one
statement, as long as it returns a String?)

You can add this definition at the top of the suite:

val isLinux = { val osName = Option(System.getProperty("os.name")) osName.exists(_.toLowerCase().contains("linux")) }

LeeTibbert · 2022-09-11T21:32:12Z

I have the runtime "os.name" test working JVM & SN on Linux. Off to
check my mac to see what it uses. I am trying not to peek at the SN code,
which I have seen a thousand times already.

As events unfold, we will probably want to move this test to
someplace common in the Suite. Or use another alternative.

…on msg text

armanbilge

Well that was quite an adventure! Thanks for chasing those down 😃

Is this ready for final review? Because it is LGTM. Merge at will :)

LeeTibbert · 2022-09-11T23:04:08Z

Thank you for the review and tutorage. I may end up learning some Cats IO.

I think this is ready for merge at your convenience.

armanbilge · 2022-09-11T23:12:53Z

I'd say you've got the hang of it, you did a very nice job with the test :)

armanbilge reviewed Sep 10, 2022

View reviewed changes

core/src/main/scala/epollcat/internal/ch/SocketHelpers.scala Outdated Show resolved Hide resolved

LeeTibbert added 4 commits September 11, 2022 15:16

Fix armanbilge#39: getaddrinfo numeric error codes are now translated

bc50c2a

Per review, do not display address & port publicaly

100a369

Align some Native & JVM Exceptions & messages

3e6323e

Scalafmt new UnsupportedAddressException

eec0fd2

Per review, add test for BindException(EADDRNOTAVAIL)

5fadc54

LeeTibbert force-pushed the PR_GaiErrorString_I39 branch from 4bc03c3 to 5fadc54 Compare September 11, 2022 20:38

armanbilge reviewed Sep 11, 2022

View reviewed changes

LeeTibbert added 2 commits September 11, 2022 17:41

BindException(EADDRNOTAVAIL) now accepts both Linux and macOS excepti…

a83e2ba

…on msg text

Fix EADDRNOTAVAIL hack for macOS. Hacks always return to bite, hard!

0bfc078

armanbilge approved these changes Sep 11, 2022

View reviewed changes

armanbilge merged commit 67ca8bc into armanbilge:main Sep 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #39: getaddrinfo numeric error codes are now translated #40

Fix #39: getaddrinfo numeric error codes are now translated #40

LeeTibbert commented Sep 10, 2022 •

edited

Loading

LeeTibbert commented Sep 10, 2022

LeeTibbert commented Sep 10, 2022

LeeTibbert commented Sep 10, 2022

armanbilge left a comment

armanbilge Sep 10, 2022

LeeTibbert Sep 10, 2022

armanbilge Sep 10, 2022

LeeTibbert Sep 10, 2022

armanbilge Sep 10, 2022

LeeTibbert Sep 10, 2022

LeeTibbert commented Sep 11, 2022

armanbilge commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

armanbilge commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

armanbilge commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

armanbilge Sep 11, 2022

LeeTibbert Sep 11, 2022

armanbilge Sep 11, 2022

LeeTibbert commented Sep 11, 2022 •

edited

Loading

armanbilge left a comment

LeeTibbert commented Sep 11, 2022

armanbilge commented Sep 11, 2022


		val port = addr.getPort.toString

		s"${gaiMsg} address: ${adr} port: ${port}"

Fix #39: getaddrinfo numeric error codes are now translated #40

Fix #39: getaddrinfo numeric error codes are now translated #40

Conversation

LeeTibbert commented Sep 10, 2022 • edited Loading

LeeTibbert commented Sep 10, 2022

LeeTibbert commented Sep 10, 2022

LeeTibbert commented Sep 10, 2022

armanbilge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeeTibbert commented Sep 11, 2022

armanbilge commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

java.nio.channels.UnsupportedAddressTypeException

EADDRNOTAVAIL

armanbilge commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

armanbilge commented Sep 11, 2022

LeeTibbert commented Sep 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeeTibbert commented Sep 11, 2022 • edited Loading

armanbilge left a comment

Choose a reason for hiding this comment

LeeTibbert commented Sep 11, 2022

armanbilge commented Sep 11, 2022

LeeTibbert commented Sep 10, 2022 •

edited

Loading

LeeTibbert commented Sep 11, 2022 •

edited

Loading