[jira] [Created] (TS-3453) Confusion of handling SSL events in write_to_net_io in UnixNetVConnection.cc

["Susan Hinrichs (JIRA)" <ji...@apache.org>]

Susan Hinrichs created TS-3453:
----------------------------------

             Summary: Confusion of handling SSL events in write_to_net_io in UnixNetVConnection.cc
                 Key: TS-3453
                 URL: https://issues.apache.org/jira/browse/TS-3453
             Project: Traffic Server
          Issue Type: Bug
          Components: SSL
            Reporter: Susan Hinrichs


While tracking down differences for SSL between 5.0 and 5.2 for TS- I came across odd event handling code in write_to_net_io in UnixNetVConnection.cc.  Looking back at the history in that code things became even more confusing.

The current version on master (same as what is in 5.2) contains the following in an if/else sequence. SSL IOs can return READ events event from write functions (and visa versa), so that is why I assume that this write function is dealing with SSL read events at all.

{code}
if (ret == SSL_HANDSHAKE_WANT_READ || ret == SSL_HANDSHAKE_WANT_ACCEPT || ret == SSL_HANDSHAKE_WANT_CONNECT
               || ret == SSL_HANDSHAKE_WANT_WRITE) {
      vc->read.triggered = 0;
      nh->read_ready_list.remove(vc);
      vc->write.triggered = 0;
      nh->write_ready_list.remove(vc);
      if (ret == SSL_HANDSHAKE_WANT_READ || ret == SSL_HANDSHAKE_WANT_ACCEPT)
        read_reschedule(nh, vc);
      else
        write_reschedule(nh, vc);
    }
{code}

Seems odd to be clearing and remove read events if the real event was only a write.  And visa versa, seems odd to be clearing and removing write events if the real event was a read.

It seems to me that the sequence should be replaced with something like

{code}
if (ret == SSL_HANDSHAKE_WANT_READ || ret == SSL_HANDSHAKE_WANT_ACCEPT) {
      vc->read.triggered = 0;
      nh->read_ready_list.remove(vc);
      read_reschedule(nh, vc);
    } else if (ret == SSL_HANDSHAKE_WANT_CONNECT || ret == SSL_HANDSHAKE_WANT_WRITE) {
      vc->write.triggered = 0;
      nh->write_ready_list.remove(vc);
      write_reschedule(nh, vc);
    }
{code}

Looking back at the history shows adding and removing and re-adding of reschedules.  

* TS-3006 9/22/14 by me.  Adds in the read_reschedule case

* TS-2815 5/16/14 by [~bcall] Removes the read_reschedule case

* TS-2211 10/28/13 by postwait Adds read_reschedule and protects the write_reschedule and read_reschedule with specific event checks.

* TS-1921 5/17/13 by [~jpeach@apache.org] Adds in the write_reschedule

This seems like an obvious tidy up thing.  I'm not addressing a specific issue here, but the current thing seems wrong.  Given the history, I'm hesitant to clean things up without review from those that came before.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)