[Live-devel] Potential memory leak and incomplete response if client disconnects while streaming HLS

[Bruce Liang]

Hi,

I'm using live555MediaServer on linux to stream HLS and I found problem in TCPStreamSink that may
cause memory leak and incomplete response.

In TCPStreamSink::processBuffer() there is a 'if' statement 'if (envir().getErrno() != EPIPE)'
that checks whether client closes connection during send() call.

After 'EPIPE' error occurs, this 'if' statement may cause memory leak and the response to client
can't be sent completely ever after, if socket send buffer is getting full during send() call(i.e.
int a = send(socknum, data_ptr, b, 0), the result is a < b and a != -1).

I'm going to explain it:

In TCPStreamSink.cpp,

void TCPStreamSink::processBuffer() {
...
    int numBytesWritten
      = send(fOutputSocketNum, (const char*)&fBuffer[fUnwrittenBytesStart], numUnwrittenBytes(), 0);
    if (numBytesWritten < (int)numUnwrittenBytes()) {
      // The output socket is no longer writable.  Set a handler to be called when it becomes writable again.
      fOutputSocketIsWritable = False;
      if (envir().getErrno() != EPIPE) { // on this error, the socket might still be writable, but no longer usable
            envir().taskScheduler().setBackgroundHandling(fOutputSocketNum, SOCKET_WRITABLE, socketWritableHandler, this);
      }
    }
...
}

1.memory leak:
If client closes connection while sending, 'numBytesWritten' will be -1 and 'errno' will be set to 'EPIPE'.

The task will not be scheduled and 'onSourceClosure()' will has no chance to be called.

Since the socket is no longer usable(according to changelog.txt on 2013.10.25), the socket and
resource for this session should be released when 'EPIPE' error occurs. (Normally, if no error occurs,
afterStreaming(in RTSPClientConnectionSupportingHTTPStreaming) is responsible for this.)

And this results in memory leak.

2.incomplete response:
The two 'if' statement in 'TCPStreamSink::processBuffer()' will make the situation that
"socket send buffer becomes full during a send() call" be considered as 'EPIPE' error,
if there were a 'EPIPE' error in previous session.

In this case, 'numBytesWritten' would not be -1 and errno will not be changed.
Since errno is still 'EPIPE', 'if (envir().getErrno() != EPIPE)' will be false and then the task won't be scheduled.
The sending task is stopped but it should actually try it later.

And this results in incomplete response.


I have considered a suggestion to this problem:

Here are 2 references for errno handling,
1.
http://man7.org/linux/man-pages/man2/send.2.html
RETURN VALUE         top
       On success, these calls return the number of bytes sent.  On error,
       -1 is returned, and errno is set appropriately.
2.
https://resources.sei.cmu.edu/downloads/secure-coding/assets/sei-cert-c-coding-standard-2016-v01.pdf
13.1 ERR30-C. Set errno to zero before calling a library function known
to set errno, and check errno only after the function returns a value
indicating failure

According to man-pages, the send() will set errno when -1 is returned.
And if errno is going to be checked, the send() needs to return a value indicating failure,
as sei-cert-c-coding-standard suggested.

The value returned by send() should also be examined for indicating that 'EPIPE' is actually
set by this send() call, not others.

'onSourceClosure()' can also be used in this situation to release resource.
Or maybe a new method that can handle resource can be defined in TCPStreamSink's scope
(onSourceClosure() is defined in MediaSink class which may cause a misunderstanding of
the purpose of the method if using it to handling EPIPE error.)

Setting errno to 0 may be unnecessary if return value of send() is examined to confirm 'EPIPE',
but it may still be worth doing it.

void TCPStreamSink::processBuffer() {
...
    errno = 0;
    int numBytesWritten
      = send(fOutputSocketNum, (const char*)&fBuffer[fUnwrittenBytesStart], numUnwrittenBytes(), 0);
    if (numBytesWritten < (int)numUnwrittenBytes()) {
      // The output socket is no longer writable.  Set a handler to be called when it becomes writable again.
      fOutputSocketIsWritable = False;
      if (!(numBytesWritten == -1 && envir().getErrno() == EPIPE)) { // on this error, the socket might still be writable, but no longer usable
            envir().taskScheduler().setBackgroundHandling(fOutputSocketNum, SOCKET_WRITABLE, socketWritableHandler, this);
      }
      else{
            onSourceClosure();
      }
    }
...
}

Or maybe a callback function could be added to TCPStreamSink(like fOnSendErrorFunc in
MultiFramedRTPSink) for notifying user that there is a 'EPIPE' error and leave the job
to release resource to be user-defined.

...
if (!(numBytesWritten == -1 && envir().getErrno() == EPIPE)) {
  envir().taskScheduler().setBackgroundHandling(fOutputSocketNum, SOCKET_WRITABLE, socketWritableHandler, this);
}
else{
  if (fOnEPIPESendErrorFunc != NULL) (*fOnEPIPESendErrorFunc)(fOnEPIPESendErrorData);
}
...

I'm wondering if there are some suggestions for handling this problem?

Thanks,

Bruce Liang
Ability Enterprise Co., Ltd
*** Confidentiality Note *** This e-mail message and any accompanying documents are for sole use of the intended recipient(s) and may contain confidential and privileged information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.live555.com/pipermail/live-devel/attachments/20200310/392f496e/attachment.htm>