TS-4697: free MIOBuffer if fails on ipallow check.

[oknet]

No description provided.

[oknet]

Due to multi types of SessionAccept, I'm change the return type of SessionAccept::accept() from void to int and free the MIOBuffer in ProtocolProbeTrampoline::ioCompletionEvent().

EVENT_DONE : ip allow check fails or forbidden, the caller should free MIOBuffer.
EVENT_CONT : The call to accept() successfully handle a session.

[zwoop]

Certainly seems reasonable to me. [approve ci]

[atsci]

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/373/ for details.

[zwoop]

@oknet This failed clang-format, please fix and push update :).

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/476/ for details.

[oknet]

clang-format fixed. @zwoop

[jpeach]

We don't really use event return values outside of event handlers, so if we return a failure from accept(), it should just be a bool.

Originally, the contract for SessionAccept::accept() was that it always consumed its arguments. This means that it should be responsible for releasing the MIOBuffer on failure. With your proposal, the caller deleted the buffer but the callee closes the vc. Both the vc and the buffer should be treated consistently, so either enforce the original contract, or delete any allocated buffers in the leaf handlers when they fail (updating the comments that describe the calling contract).

[oknet]

@jpeach SessionAccept is a interface to create ClientSession. Its mutex is NULL, it is not safe to release any resource. The caller to SessionAccept is Trampline that mutex is copy from NetVC, it is safe to handle resource release. This is why I'm free MIOBuffer in Trampline.

[jpeach]

You don't need to hold a mutex to free anything. In all of these cases, the callee is supposed to consume both parameters. For consistency, we should not sometime consume only some of the parameters.

[oknet]

@jpeach agree with u to return bool from SessionAccept::accept().

SessionAccept did not consume any parameters, It is just transfer parameters to ClientSession.
as your said "Both the vc and the buffer should be treated consistently", I'm prefer to move do_io_close() and free MIOBuffer from SessionAccept to ProtocolProbeTrampoline.

Just return false if ipallow check fails in XXXSessionAccept::accept().
Then, the NetVC and MIOBuffer will be close&free in ProtocolProbeTrampoline.

[jpeach]

Thanks @oknet. Can you please document the invariants for accept in I_SessionAccept.h?

[oknet]

@jpeach Do you means L175 ~ L184 in proxy/http/HttpSessionAccept.h ?

175 /**
176    The continuation mutex is NULL to allow parellel accepts in NT. No
177    state is recorded by the handler and values are required to be set
178    during construction via the @c Options struct and never changed. So
179    a NULL mutex is safe.
180 
181    Most of the state is simply passed on to the @c ClientSession after
182    an accept. It is done here because this is the least bad pathway
183    from the top level configuration to the HTTP session.
184 */
185 
186 class HttpSessionAccept : public SessionAccept, private detail::HttpSessionAcceptOptions

Just copy the comment from HttpSessionAccept.h to I_SessionAccept.h maybe with minor changes ?

[jpeach]

Yeh that sounds reasonable. I'd like the comment to give accurate information about when the arguments need to be deleted and when the callee takes ownership.

[oknet]

@jpeach comments added, please review.

[jpeach]

[approve ci]

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/556/ for details.

[atsci]

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/452/ for details.