channel: remove unused code

[d3x0r]

Nobody missed it before; why would they now?

Ref #1344.

Was going through the code and found some unused code.

https://github.com/libssh2/libssh2/blob/master/src/channel.c#L2009-L2017

if channel->extData2_state == libssh2_NB_state_idle when the function starts, then the first if changes it to libssh2_NB_state_created and then for the next if the state won't be idle anymore, and the second if won't run. If it's not idle, then nothing happens, and the state is reset to idle.

Really this is the only function that uses extData2_state

So the function becomes just

        _libssh2_debug((channel->session, LIBSSH2_TRACE_CONN,
                       "Setting channel %u/%u handle_extended_data"
                       " mode to %d",
                       channel->local.id, channel->remote.id, ignore_mode));
        channel->remote.extended_data_ignore_mode = (char)ignore_mode;

and extData2_state can be removed from the channel structure.

---

Or - maybe if there is really a flush still desired, maybe the second if should use channel->extData2_state == libssh2_NB_state_created instead of channel->extData2_state == libssh2_NB_state_idle.

[vszakats]

@libssh2/dev-team: Any opinion on this PR? Are we safe to merge?

[MichaelBuckley]

I'm trying to be paranoid about innocent-looking changes because of the recent xz exploit, but this seems OK to me. The only way this could affect execution is if extData2_state was not set to libssh2_NB_state_idle at the start of the function. But this is the only function that explicitly sets extData2_state, and libssh2_NB_state_idle is 0.

So potentially you could get into trouble if your LIBSSH2_CHANNEL was not initialized to 0, but _libssh2_channel_open creates the channel using LIBSSH2_CALLOC which is defined to _libssh2_calloc, which zeros out the entire structure.

Still being paranoid, maybe there's a memcpy or a memmove that overwrites that part of the LIBSSH2_CHANNEL struct, or maybe d3x0r knows about a popular client that redefines LIBSSH2_CALLOC or accidentally overwrites memory so that extData2_state is nonzero, and thus extended_data_ignore_mode does not get set the first time this function is called. That cause the client to either read or not read SSH_MSG_CHANNEL_EXTENDED_DATA messages when it would in the past do the opposite, but I'm not sure how that leads to an exploit.

I think this change is probably fine, assuming clients aren't overwriting data in LIBSSH2_CHANNEL, intentionally or otherwise.

[vszakats]

Thanks @MichaelBuckley.

Your assessment, and the original proposition are giving the impression that this may add some potential risk and/or reduce robustness, while it's not completely certain if it resolves an actual issue. For the gain of 15 lines of code and saving one member in the channel structrure, I think it's not worth taking that risk.

What about adding a comment on the top of this block of code instead, that explains the situation for future readers?

[MichaelBuckley]

I think the risk is very, very, very small. If the client is using libssh2 correctly, there is no risk. I am only being paranoid because of the recent xz exploit.

That said, you are right that the benefit is also small. 15 lines of code, 1 strict member, and a small amount of execution time.