Opened 6 years ago

Closed 5 years ago

#946 closed defect (fixed)

Bind error in Tunnel Manager when destination address is invalid

Reported by: guest Owned by: zzz
Priority: minor Milestone: 0.9.8
Component: apps/i2ptunnel Version: 0.9.6
Keywords: Cc:
Parent Tickets:

Description

Steps to reproduce in i2ptunnel manager:

  • Create a standard tunnel with default settings. Make sure autostart is enabled
  • Include a bogus destination address - we use bugs.are.cool.i2p
  • Start the tunnel
  • Here confirm errors. No bind error is present. We know target destination is not found
  • Tunnel will be 'not started' a few moments after autostart failure - Start the tunnel now
  • Note bind errors on interface, address/port already in use

Eventually the tunnel starts and the bind error goes away.
I presume this is due to after initial failure bind is not destroyed correctly.

Subtickets

Change History (9)

comment:1 Changed 6 years ago by str4d

See apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelClient.java lines 51-66:

if (dests.isEmpty()) {
    l.log("No valid target destinations found");
    notifyEvent("openClientResult", "error");
    // Nothing is listening for the above event, so it's useless
    // Maybe figure out where to put a waitEventValue("openClientResult") ??
    // In the meantime, let's do this the easy way
    // Note that b32 dests will often not be resolvable at instantiation time;
    // a delayed resolution system would be even better.

    // Don't close() here, because it does a removeSession() and then
    // TunnelController can't acquire() it to release() it.
    //close(true);
    // Unfortunately, super() built the whole tunnel before we get here.
    throw new IllegalArgumentException("No valid target destinations found");
    //return;
}

comment:2 Changed 6 years ago by str4d

FindBugs? doesn't like this either - http://jenkins.killyourtv.i2p/job/I2P/202/findbugsResult/file.147862710/

I2PTunnelClientBase.java:115, SC_START_IN_CTOR, Priority: High

SC: new net.i2p.i2ptunnel.I2PTunnelClientBase(int, Logging, I2PSocketManager, I2PTunnel, EventDispatcher?, long) invokes Thread.start()

The constructor starts a thread. This is likely to be wrong if the class is ever extended/subclassed, since the thread will be started before the subclass constructor is started.

Question is, what to do about it? At what stage should the local IP:port be set up, and by what? I see there are two almost-identical constructors in I2PTunnelClientBase, one of which respects a "delayOpen" parameter, but that only delays opening the I2P side of the tunnel.

comment:3 Changed 6 years ago by zzz

Related: #651 #722, possibly others. One of them mentions the start-thread-in-constructor issue.

If the tunnel is a shared client it may be even worse, see e.g. #815

I2PTunnelDCCClient delays resolution until connect time. I think at one point I intended on porting that "up" to I2PTunnelClient (and some of the other subclasses) but never did it.

Perhaps because maybe we do want to complain or fail if it doesn't resolve? But maybe only if it's a "real" hostname, but not a well-formed b32. So I guess I never did it because I never decided what to do exactly.

The thread issue is really bad but it may or may not be necessary to fix it to fix this ticket. I have 30 things in i2ptunnel.config, all but a couple are disabled, but we start a thread for each of them. The whole event and notify thing is pretty whacky.

comment:4 Changed 6 years ago by zzz

  • Milestone changed from 0.9.7 to 0.9.8
  • Owner set to zzz
  • Status changed from new to accepted

The above comment isn't completely true, we don't start a thread for each tunnel anymore.

Working on fixing all this in the i2p.i2p.zzz.test2 branch.

comment:5 Changed 5 years ago by guest

  • Resolution set to worksforme
  • Status changed from accepted to closed

comment:6 Changed 5 years ago by str4d

  • Resolution worksforme deleted
  • Status changed from closed to reopened

comment:7 Changed 5 years ago by guest

  • Resolution set to duplicate
  • Status changed from reopened to closed

comment:8 Changed 5 years ago by str4d

  • Resolution duplicate deleted
  • Status changed from closed to reopened

comment:9 Changed 5 years ago by zzz

  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed in standard and IRC client tunnels in 0.9.8.1-8 3ddedb08de8709fb784e8557b80feefe0d1ddf8a. Didn't bother to fix streamr client.

Unresolvable hostname spits out a warning but does not throw out of the constructor. An unresolved hostname will be re-resolved at connection time. This also fixes the issue with b32 hostnames. Removed GUI warning that they weren't recommended.

I opened #973 a few months ago for the generic start-threads-in-constructors issue raised in comments 2 and 3 above.

Note: See TracTickets for help on using tickets.