After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 739983 - tls: deal with gnutls's internal buffering of un-sent data
tls: deal with gnutls's internal buffering of un-sent data
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: network
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-11-11 20:53 UTC by Aleix Conchillo Flaqué
Modified: 2018-05-29 06:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add flush support (3.85 KB, patch)
2014-11-11 20:53 UTC, Aleix Conchillo Flaqué
rejected Details | Review

Description Aleix Conchillo Flaqué 2014-11-11 20:53:38 UTC
Created attachment 290434 [details] [review]
add flush support

This comes from bug 739799.

GStreamer sometimes doesn't resend the same data even after receiving G_IO_ERROR_WOULD_BLOCK. This is in gst_rtsp_watch_set_flushing.

It then tries to send new data on the stream. However, GnuTLS is still expecting the same old data that couldn't be sent. This causes GnuTLS to return the old size instead of the new requested size (because it has sent the buffered data).

Implementing the flush method would solve this problem as GStreamer could call g_output_stream_flush before trying to send any new data.

May be this is all wrong and GStreamer should really send the old data instead of discarding it.

From the commit log:

    GnuTLS buffers already encrypted data in gnutls_record_send when
    returning EAGAIN or EINTR. This means a user can call again
    gnutls_record_send with NULL and 0 and the previous data will be
    sent.
    
    This patch implements GOutputStream flush method by calling
    gnutls_record_send with NULL and 0 and thus making sure no data is
    buffered by GnuTLS.
Comment 1 Aleix Conchillo Flaqué 2014-11-13 01:53:22 UTC
As a side note. Patch for this bug and bug 739799 have been tested over ~300k connection/disconnection successfully.
Comment 2 Dan Winship 2014-11-13 16:06:50 UTC
Comment on attachment 290434 [details] [review]
add flush support

GTlsOutputStreamGnutls is violating the semantics of GOutputStream; if it claims to have not written the data, then it can't write it later without being asked to. GTlsConnectionGnutls needs to hide this behavior from higher levels somehow.
Comment 3 Dan Winship 2014-11-13 16:09:51 UTC
more generally, this patch doesn't really fix the problem, since there is no way that the caller can know for sure when it needs to call g_output_stream_flush() to fix the stream state
Comment 4 Aleix Conchillo Flaqué 2014-11-13 23:24:03 UTC
(In reply to comment #2)
> (From update of attachment 290434 [details] [review])
> GTlsOutputStreamGnutls is violating the semantics of GOutputStream; if it
> claims to have not written the data, then it can't write it later without being
> asked to. GTlsConnectionGnutls needs to hide this behavior from higher levels
> somehow.

Yes, that's right. Whoever is responsible for handling the error should resend the data. This is not happening right now, and new data is being sent, which breaks everything.

(In reply to comment #3)
> more generally, this patch doesn't really fix the problem, since there is no
> way that the caller can know for sure when it needs to call
> g_output_stream_flush() to fix the stream state

That's right too. It fixes my particular problem, but not the general one.

However, glib documentation is not really clear about what to do.

https://developer.gnome.org/gio/stable/GPollableOutputStream.html#g-pollable-output-stream-write-nonblocking

Says:

  If stream is not currently writable, this will immediately return 
  G_IO_ERROR_WOULD_BLOCK, and you can use 
  g_pollable_output_stream_create_source() to create a GSource that will be 
  triggered when stream is writable.

But it doesn't say that you *must* write the same data again. Not doing it, in glib-networking gnutls case, breaks things.
Comment 5 Dan Winship 2014-11-14 00:47:33 UTC
(In reply to comment #4)
> Yes, that's right. Whoever is responsible for handling the error should resend
> the data. This is not happening right now, and new data is being sent, which
> breaks everything.

The fact that the "flush" works seems to suggest that whoever is on the other end of the connection does want/expect that data too, right? So you should be retrying? (Or is it just that things work out at the RTSP layer whether or not that data gets sent?)

>   If stream is not currently writable, this will immediately return 
>   G_IO_ERROR_WOULD_BLOCK, and you can use 
>   g_pollable_output_stream_create_source() to create a GSource that will be 
>   triggered when stream is writable.
> 
> But it doesn't say that you *must* write the same data again. Not doing it, in
> glib-networking gnutls case, breaks things.

You're not supposed to have to write the same data again.

I'm not sure what the fix/workaround here is though...
Comment 6 Aleix Conchillo Flaqué 2014-11-14 21:16:25 UTC
(In reply to comment #5)
> 
> The fact that the "flush" works seems to suggest that whoever is on the other
> end of the connection does want/expect that data too, right? So you should be
> retrying? (Or is it just that things work out at the RTSP layer whether or not
> that data gets sent?)
> 

Right, things keep working becasue this happens in the TEARDOWN request. So, the other end it's just waiting for the TEARDOWN response, if any.

To put you in context. This problem only occurs when using RTSP interleaved data. That is, RTP data that is sent over the RTSP channel (so, over TCP). Then, both RTSP messages + data are sent on the same stream.

Because we are sending quite some data over this channel, GnuTLS might return EAGAIN. This is OK before getting a TEARDOWN request, because the data not sent is retried (as expected).

However, when a TEARDOWN request is received GStreamer discards data that has not been sent and as a final message it sends the TEARDOWN response. This is the one that makes everything go wrong, because of the GnuTLS buffering.

> 
> You're not supposed to have to write the same data again.
> 
> I'm not sure what the fix/workaround here is though...

For the case described above, the solution would be, I guess, to force GStreamer send the data again.

But, as you mentioned, it is still not clear what to do in the general case considering GnuTLS constraint.

May be a new GError. Something like G_IO_ERROR_MUST_RESEND that would be returned for cases like GnuTLS. At least, this way, the user would know what to do before sending new data. Not sure if I like it much. Just a first idea.
Comment 7 Aleix Conchillo Flaqué 2014-11-16 23:10:39 UTC
(In reply to comment #6)
> 
> May be a new GError. Something like G_IO_ERROR_MUST_RESEND that would be
> returned for cases like GnuTLS. At least, this way, the user would know what to
> do before sending new data. Not sure if I like it much. Just a first idea.

This is not even backwards compatible. So it probably doesn't make any sense.
Comment 8 Michael Gratton 2018-01-04 23:09:29 UTC
Geary is encountering something similar over in Bug 792219, except that this occurs when the underlying network connection has been dropped. I thought this was perhaps a bug in GBufferedOutputStream since it is attempting to flush a stream that is closed, but the underlying GTlsOutputStream is reporting that it is not yet closed, so GBufferedOutputStream would not be able to determine that it shouldn't be flushing the stream.

From the stack trace in Bug 792219:

> (gdb) up
> #8  0x00007ffff7a7572a in flush_buffer_thread (task=0x55555a4c92c0 [GTask], object=0x55555b644f50, task_data=0x55555c089a50, cancellable=0x0)
>    at ../../../../gio/gbufferedoutputstream.c:664
> 664	in ../../../../gio/gbufferedoutputstream.c
> (gdb) info locals
> base_stream = 0x55555a530cc0 [GTlsOutputStreamGnutls]
> fdata = 0x55555c089a50
> res = <optimised out>
> error = 0x0
> (gdb) print base_stream.priv.closed 
> $3 = 0
>(gdb) print base_stream.priv.closing 
> $4 = 0
Comment 9 Michael Catanzaro 2018-02-12 02:30:07 UTC
(In reply to Dan Winship from comment #5)
> You're not supposed to have to write the same data again.
> 
> I'm not sure what the fix/workaround here is though...

I don't see any possible solution, other than to change GnuTLS to eliminate the requirement to resend the same data, or to document this limitation in GOutputStream.
Comment 10 Michael Catanzaro 2018-02-12 02:30:34 UTC
(In reply to Michael Gratton from comment #8)
> Geary is encountering something similar over in Bug 792219, except that this
> occurs when the underlying network connection has been dropped.

Your issue is unrelated.
Comment 11 GNOME Infrastructure Team 2018-05-24 17:12:43 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/956.
Comment 12 Michael Catanzaro 2018-05-25 16:35:38 UTC
Reopening. This bug was migrated to https://gitlab.gnome.org/GNOME/glib, but it should have been migrated to https://gitlab.gnome.org/GNOME/glib-networking.
Comment 13 Christoph Reiter (lazka) 2018-05-29 06:39:43 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib-networking/issues/15.