rtspconnection: GError set over the top of a previous GError or uninitialized memory.

Nils Öhnell submitted a patch:

The proposed patch goes to error not only when returned number of bytes is non-negative but also when err is set.
This change required some rework of the error handling.
The function write_bytes() is aligned to the read_byte() function for handling the case where returned bytes is zero.

~~Patch 371454~~, "Patch to address the problem":
0001-gstrtspconnection-handle-err-at-read-and-write.patch

Nils Öhnell submitted a patch:

Same comment as for obsoleted attachment 371454.
The only difference is an updated print format specifier.

~~Patch 371457~~, "Patch to address the problem":
0001-gstrtspconnection-handle-err-at-read-and-write.patch

Nils Öhnell uploaded an attachment:

Problem with rtsph detected.
The current patch is obsoleted.
A new patch will be provided when verified.

~~Attachment 371668~~, "Obsolete patch 371457":
dummyAttachment.txt

Nils Öhnell submitted a patch:

The proposed patch goes to error handling not only based on the return value from functions but also when err is set.

The change required some rework of the error handling.

Patch 372114, "gstrtspconnection.c: Error handling in write_bytes() and read_bytes()":
0001-gstrtspconnection-handle-err-at-read-and-write_new-4890a850e1c9f6e499eda4a4f7038830.patch

Nils Öhnell said:

Has anyone had a chance to look at this yet?

Tim Müller @tpm said:

Right, so I see that the warning/critical needs fixing, and I understand how that happens.

What is not entirely clear to me yet if this is the right change.

What error is being returned in this case where it returns a non-negative number as well?

Is this intentional and expected or perhaps a bug in the underlying code in GLib?

Nils Öhnell said:

What error is being returned in this case where it returns a non-negative number as well?
The returned error is depending on the G_IO_ERROR with GST_RTSP_ESYS as default, as before.

Is this intentional and expected or perhaps a bug in the underlying code in GLib?
Well, the GLib documentation states "the number of bytes ..., or -1 on error" for both the underlying GLib functions (g_pollable_output_stream_write_nonblocking and g_pollable_input_stream_read_nonblocking), so at least one could say that the GLib implementation does not match the GLib documentation.

Sebastian Dröge @slomo said:

(In reply to Nils Öhnell from comment 7)

What error is being returned in this case where it returns a non-negative number as well?
The returned error is depending on the G_IO_ERROR with GST_RTSP_ESYS as
default, as before.

I think the question was what the actual GIO error is that happens here

Is this intentional and expected or perhaps a bug in the underlying code in GLib?
Well, the GLib documentation states "the number of bytes ..., or -1 on
error" for both the underlying GLib functions
(g_pollable_output_stream_write_nonblocking and
g_pollable_input_stream_read_nonblocking), so at least one could say that
the GLib implementation does not match the GLib documentation.

Can you file a bug about that again GLib? This is definitely not correct, both for the given documentation and the behaviour that code relies on everywhere with regards to these functions.

Does this happen with direct TCP connections, or via TLS, or ...?

Hi,

I working together with Nils and I had another look at this issue. It don't think this is a Glib issue. The interesting function in this case is gst-plugins-base/gst-libs/gst/rtsp/gstrtspconnection.c: fill_bytes()

This function may return both data and error in some cases. It happens when there are leftover bytes and the next call to fill_raw_bytes() fails.

It happens in this order:

out is incremented with leftover bytes
fill_raw_bytes() sets Gerror and returns -1
The function fill_bytes() returns out containing the leftover bytes together with GError set.

In our case GError is: Resource temporarily unavailable

What should happen to the leftover data when the call to fill_raw_bytes() fails?

Based on this information I can think of two alternatives:

In read_bytes() handle the case of both returned leftover bytes and error from fill_bytes()
Change fill_bytes() to always return -1 in case of error (this would discard the leftover data)

Which alternative is preferred in this case?

The function write_bytes() is not affected by this issue. No need to change this function.

Br Per

In our case GError is: Resource temporarily unavailable

This should cause an immediate retry in this function instead of returning an error upwards.

What should happen to the leftover data when the call to fill_raw_bytes() fails?

If there's a real error (not EWOULDBLOCK, not EINTR/EAGAIN) then it should error out immediately and whatever data was read is discarded.

On EWOULDBLOCK you would return all data and no error (and the next call would directly return EWOULDBLOCK without any data), on EINTR/EAGAIN you would internally retry and never bother the caller with that detail.

Thanks for super fast response.

I'm not really found of having an internal retry mechanism. I'm afraid it could cause more harm than good due to added complexity.

It sounds simple to always return leftover data (if any) and discard error. The caller will call again to read more bytes (implicit retry mechanism) and if error is persistent it will fail again.

Is it really safe to discard the leftover bytes?

Don't they belong to a previous successful call.

I'm not really found of having an internal retry mechanism. I'm afraid it could cause more harm than good due to added complexity.

I remember it being already there for the write code at least. It's better to keep this internally instead of having all callers worry about that.

Is it really safe to discard the leftover bytes?

Don't callers expect to get as many bytes as they requested, and otherwise error out anyway? Please check the callers :)

Thanks for the feedback and discussion today.

I don't have the possibility to pick up this work right away now but at least we know have a better understanding of the issue now.

Next step is to propose a new patch.

To be continued ...

Currently the write functions are handling retries internally, I'd prefer to have the read functions work the same for consistency and to keep this kind of problems from happening.