assert failures on debug build

[bendikro]

When testing a libtorrent debug build there are some assert failures due to invalid input.

These are from libtorrent 1.0.3:

• Assertion failed: (max_connections >= 2 || max_connections == -1), function set_max_connections, file torrent_handle.cpp, line 296.
• Assertion failed: (ec != error::invalid_argument || !m_outgoing), function disconnect, file peer_connection.cpp, line 3553.
• Assertion failed: ((p == -1) == is_finished() || (!m_auto_managed && p == -1) || (m_abort && p == -1)), function set_queue_position, file torrent.cpp, line 6973.

Would it be possible to not have these as asserts? Asserts shouldn't really be failing on invalid input from external callers, but for incorrect logic in the internal code?

Also, torrent_handle::set_max_connections fails when max_connections is 0 or 1, however, torrent::set_max_connections uses

TORRENT_ASSERT(limit >= -1);
if (limit <= 0) limit = (1<<24)-1;

which implies that 0 is valid input and is treated as unlimited together with -1.

What happens when running a release build and torrent_handle::set_max_connections is called with max_connections=1? Will it accept 1 even though the docs says it must be either -1 or >= 2 ?

[arvidn]

the middle assertion, checking for invalid_argument, actually implies a bug somewhere. Do you know why it happens? Why would libtorrent pass in invalid arguments? The one legitimate case I know of is when trying to bind an outgoing socket to a port that's already in use (which is fine) and then connect it to the same destination IP and port (which is not OK, the quadruple of IP and port pairs must be unique). That's why there's an exception for outgoing connections.

I think in some places I have combined asserts for invalid input with runtime checks afterwards, to abort. It's a bit cleaner in master I believe. If you have any specific suggestions, please post a pull request!

[arvidn]

I also believe 0 is more well defined in master as well, to actually mean 0

[bendikro]

Ah, the peer_connection::disconnect assert has been commented out, I missed that when looking at the latest code. I don't know why it would be given invalid arguments, but are you saying that the current code (by design) allows a user to cause a situation where disconnect is given invalid input (by doing something silly) ? Or is it supposed to be logically impossible?

For other asserts, such as in torrent_handle::set_max_connections, I was thinking something like this:

    void torrent_handle::set_max_connections(int max_connections) const
    {
-       TORRENT_ASSERT_PRECOND(max_connections >= 2 || max_connections == -1);
+       if (max_connections == 1 || max_connections < -1) {
+#ifndef TORRENT_DISABLE_LOGGING
+           debug_log("Invalid input to set_max_connections: %d", max_connections);
+#endif
+           return;
+       }
        TORRENT_ASYNC_CALL2(set_max_connections, max_connections, true);
    }

Is there another preferred way to produce such as message?

It's still not clear to me what the valid inputs to set_max_connections are. 0 is handled by torrent::set_max_connections (to be the same as -1), but torrent_handle::set_max_connections will assert on 0. The docs says the value must be at least 2 or -1, but only torrent::set_max_connections will assert on 1, as torrent::set_max_connections fails only on values less than -1. As far as I can see 1 will be accepted by both (in release), even though the docs says it is invalid.

[arvidn]

TORRENT_ASSERT_PRECOND() is an assert that preconditions are met when calling this function. It's intended to fail fatally to make it obvious to the programmer that the input is invalid, and break the debugger to make it trivial to see where the call was made and the whole program state.

The check is disabled in release builds and (supposed to be) caught by a runtime check that silently ignores the invalid input. I don't see how logging it is preferable. It would make it be ignored by default and may leave bugs go unnoticed.

Now, obviously it's important that the precondition check matches the documentation. Setting the number of connections to 0 or 1 would probably increase complexity of the API, because it would in effect pause the torrent, meaning there would be multiple distinct states a torrent could be in when paused (and the "paused" state would only be one of them)