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 677560 - rtpjpegdepay: crash in copy_into_unchecked
rtpjpegdepay: crash in copy_into_unchecked
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.31
Other Linux
: Normal major
: 1.0.7
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-06-06 15:58 UTC by Álvaro López López
Modified: 2013-04-26 17:47 UTC
See Also:
GNOME target: ---
GNOME version: 2.29/2.30


Attachments
copy_into_unchecked revision to avoid segmentation faults (1.02 KB, patch)
2012-09-04 07:46 UTC, Álvaro López López
rejected Details | Review
Check for avail >= 2 in gstrtpjpegdepay (1.05 KB, patch)
2013-04-22 06:32 UTC, Petteri Aimonen
rejected Details | Review

Description Álvaro López López 2012-06-06 15:58:05 UTC
Segmentation fault on streaming application.

GstPipelines initialized using:

gst_parse_launch(
"rtspsrc name=rtspsrc ! queue ! rtpjpegdepay ! ffdec_mjpeg ! ffmpegcolorspace ! xvimagesink name=videosink",NULL);

gdb backtrace:
(gdb) backtrace
  • #0 copy_into_unchecked
    at gstadapter.c line 250
  • #1 gst_rtp_jpeg_depay_process
    at gstrtpjpegdepay.c line 643
  • #2 gst_base_rtp_depayload_chain
    at gstbasertpdepayload.c line 345
  • #3 gst_pad_chain_data_unchecked
    at gstpad.c line 4122
  • #4 gst_pad_push_data
    at gstpad.c line 4351
  • #5 gst_queue_push_one
    at gstqueue.c line 1083
  • #6 gst_queue_loop
    at gstqueue.c line 1185
  • #7 gst_task_func
    at gsttask.c line 238
  • #8 default_func
    at gsttaskpool.c line 70
  • #9 g_thread_pool_thread_proxy
    at /build/buildd/glib2.0-2.24.1/glib/gthreadpool.c line 315
  • #10 g_thread_create_proxy
    at /build/buildd/glib2.0-2.24.1/glib/gthread.c line 1893
  • #11 __pthread_initialize_minimal_internal
    from /lib/libpthread.so.0
  • #12 tcsetattr
    from /lib/libc.so.6

Comment 1 Álvaro López López 2012-06-08 08:20:31 UTC
Gstreamer 0.10.28 packages installed under Ubuntu Lucid Lynx
Comment 2 Tim-Philipp Müller 2012-06-08 08:30:24 UTC
Perhaps you could try with a more recent version of Ubuntu and GStreamer? (that is: core/base 0.10.36, -good 0.10.31, etc.)
Comment 3 Álvaro López López 2012-06-08 08:52:15 UTC
I'll try newer versions as soon as possible.
I have installed:

- gnonlin-0.10.17
- gst-ffmpeg-0.10.11
- gst-plugins-base-0.10.36
- gst-plugins-good-0.10.31
- gstreamer-0.10.36

I have had problems configuring gst-ffmpeg-0.10.12 and 13....
Thanks in advance,

Alvaro
Comment 4 Tim-Philipp Müller 2012-06-08 08:56:14 UTC
Thanks!

Since the crash seems to be in the depayloader and not in the decoder, the older version of gst-ffmpeg might be okay as well. Or you could try jpegdec instead of ffdec_mjpeg. Or just put a fakesink after the depayloader.
Comment 5 Álvaro López López 2012-06-08 09:03:46 UTC
Than you very much Tim, I'll try your options as soon as possible.
Comment 6 Álvaro López López 2012-06-26 15:49:26 UTC
I still get a segmentation fault with copy_into_unchecked using GStreamer 0.10.36...(packages version in a previous message 2012-06-08...).

I get the following gdb backtrace:

  • #0 copy_into_unchecked
    at gstadapter.c line 262
  • #1 gst_rtp_jpeg_depay_process
    at gstrtpjpegdepay.c line 678
  • #2 gst_base_rtp_depayload_chain
    at gstbasertpdepayload.c line 350
  • #3 gst_pad_push
    at gstpad.c line 4710
  • #4 gst_proxy_pad_chain_default
    at gstghostpad.c line 261
  • #5 gst_pad_push
    at gstpad.c line 4710
  • #6 gst_proxy_pad_chain_default
    at gstghostpad.c line 261
  • #7 gst_pad_push
    at gstpad.c line 4710
  • #8 gst_rtp_pt_demux_chain
    at gstrtpptdemux.c line 391
  • #9 gst_pad_push
    at gstpad.c line 4710
  • #10 gst_rtp_jitter_buffer_loop
    at gstrtpjitterbuffer.c line 1916
  • #11 gst_task_func
    at gsttask.c line 327
  • #12 default_func
    at gsttaskpool.c line 70
  • #13 ??
    from /lib/libglib-2.0.so.0
  • #14 ??
    from /lib/libglib-2.0.so.0
  • #15 start_thread
    from /lib/tls/i686/cmov/libpthread.so.0
  • #16 clone
    from /lib/tls/i686/cmov/libc.so.6

Comment 7 Álvaro López López 2012-06-26 15:52:32 UTC
Components used (now without a queue):

rtspsrc ! rtpjpegdepay ! ffdec_mjpeg ! ffmpegcolorspace ! xvimagesink
Comment 8 Álvaro López López 2012-07-16 11:06:12 UTC
It also takes place with jpegdec as in:

rtspsrc name=rtspsrc ! queue ! rtpjpegdepay ! jpegdec ! xvimagesink name=videosink

Is it possible to do something? 

I attach again the backtrace:

  • #0 copy_into_unchecked
    at gstadapter.c line 262
  • #1 gst_rtp_jpeg_depay_process
    at gstrtpjpegdepay.c line 678
  • #2 gst_base_rtp_depayload_chain
    at gstbasertpdepayload.c line 350
  • #3 gst_pad_push
    at gstpad.c line 4710
  • #4 gst_queue_push_one
    at gstqueue.c line 1156
  • #5 gst_queue_loop
    at gstqueue.c line 1264
  • #6 gst_task_func
    at gsttask.c line 327
  • #7 default_func
    at gsttaskpool.c line 70
  • #8 ??
    from /lib/libglib-2.0.so.0
  • #9 ??
    from /lib/libglib-2.0.so.0
  • #10 start_thread
    from /lib/tls/i686/cmov/libpthread.so.0
  • #11 clone
    from /lib/tls/i686/cmov/libc.so.6

Comment 9 Álvaro López López 2012-09-03 16:58:09 UTC
This changes made the trick at gstreamer-0.10.36/libs/gst/base/gstadapter.c:

/* copy data into @dest, skipping @skip bytes from the head buffers */
static void
copy_into_unchecked (GstAdapter * adapter, guint8 * dest, guint skip,
    guint size)
{
  GSList *g;
  GstBuffer *buf;
  guint bsize, csize;

  /* first step, do skipping */
  /* we might well be copying where we were scanning */
  if (adapter->priv->scan_entry && (adapter->priv->scan_offset <= skip)) 
  {
    g = adapter->priv->scan_entry;
    skip -= adapter->priv->scan_offset;
  }
  else 
  {
    g = adapter->buflist;
  }

  buf = g->data;
  bsize = GST_BUFFER_SIZE (buf);
  
  while (G_UNLIKELY (skip >= bsize)) 
  {
    		skip -= bsize;
		g = g_slist_next (g);
		/********************/
		if(g != NULL)
		{
	    		buf = g->data;
			bsize = GST_BUFFER_SIZE (buf);
		}
		else
			break;
		/********************/

    
  }
  /* copy partial buffer */
  csize = MIN (bsize - skip, size);
  memcpy (dest, GST_BUFFER_DATA (buf) + skip, csize);
  size -= csize;
  dest += csize;

  /* second step, copy remainder */
  while (size > 0) 
  {
    g = g_slist_next (g);
	/********************/
	if(g != NULL)
	{	
	    buf = g->data;
	    bsize = GST_BUFFER_SIZE (buf);
		if (G_LIKELY (bsize > 0)) 
		{
			csize = MIN (bsize, size);
	      		memcpy (dest, GST_BUFFER_DATA (buf), csize);
	      		size -= csize;
	      		dest += csize;
		}
	}
	else 
		break;
	/********************/
  }
}
Comment 10 Tim-Philipp Müller 2012-09-03 18:01:49 UTC
Thanks for tracking this down, but could you attach a proper patch please?

Preferably in git format-patch format, but git diff or diff -u will do as well. And could you describe the rationale for the change?

Thanks!
Comment 11 Tim-Philipp Müller 2012-09-03 18:02:35 UTC
And could you give us your full name please, so we can attribute the patch properly? (ideally change your bugzilla preferences)
Comment 12 Álvaro López López 2012-09-04 07:46:44 UTC
Created attachment 223399 [details] [review]
copy_into_unchecked revision to avoid segmentation faults

I have added a check to the GSList *g pointer after the call to g_slist_next(g) in order to avoid access to its data property (g->data) when pointing to NULL.
Comment 13 Álvaro López López 2012-09-04 07:48:59 UTC
Please, check the patch. I have also changed my preferences at bugzilla. 
Thanks again.
Comment 14 Tim-Philipp Müller 2012-10-19 17:38:21 UTC
Thanks for the update. It's not entirely clear to me yet what's going on here exactly. At first glance it looks like it shouldn't happen that the list nodes are NULL there.

Any chance you could you capture some of the jpeg RTP stream and make it available somewhere, so we can try and reproduce it?

   gst-launch-0.10 -e rtspsrc location=rtsp://....  ! gdppay ! filesink location=jpeg-rtp.gdp

Then just press control-C after a while. (I don't know how long it takes for the crash to happen - does it crash immediately or only after hours or .. ?)
Comment 15 Tim-Philipp Müller 2012-11-24 15:18:43 UTC
Any chance you could capture a stream like described in the previous comment, Álvaro?
Comment 16 Tobias Mueller 2013-03-10 13:52:53 UTC
Álvaro, ping
Comment 17 Álvaro López López 2013-03-11 09:23:15 UTC
Hello,

Sorry for the delay. For the moment I cannot make a capture like the one you asked for :(

What I can say is that it was not a deterministic crash. It sometimes happened from time to time and sometimes I had few crashes in a while.

After patching that library I have not noticed any strange effect any more so I´m afraid sometimes the pointer was NULL. As you didn´t check it, it would crash.

Maybe my patch is incomplete or has an error and you should include any other code... for me, GStreamer is now working OK :)
Comment 18 Álvaro López López 2013-03-21 11:38:58 UTC
I forgot to mention the IP cameras used in my tests (in case you have the opportunity to try them):

- Axis: M3113R-M12 and M3114-R

- Vivotek: MD7560 and MD8562

The crash happened with H.264 and MJPEG traffic.
Comment 19 Petteri Aimonen 2013-04-22 06:32:41 UTC
Created attachment 242118 [details] [review]
Check for avail >= 2 in gstrtpjpegdepay

I have run into this same bug when streaming data gstreamer -> gstreamer with MJPEG encoding over RTP & UDP.

To me, it seems that this bug occurs when 'avail' in gst_rtp_jpeg_depay_process is less than 2. Then skip = -1, which causes the crash in copy_into_unchecked. This can be seen in the backtrace as skip = 4294967294.

I think the attached patch should be more reasonable than the one proposed earlier. These 0- or 1-byte images are obviously corrupted, but gstreamer should still be able to continue and not crash.
Comment 20 Sebastian Dröge (slomo) 2013-04-22 08:20:36 UTC
Instead of inserting EOI there the frame should just be dropped:

commit fdb667ae00401e01fa9e014995bce541d8cec288
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Apr 22 10:19:29 2013 +0200

    rtpjpegdepay: Drop frame if it's less than 2 bytes large
    
    https://bugzilla.gnome.org/show_bug.cgi?id=677560
Comment 21 Álvaro López López 2013-04-25 08:02:51 UTC
It is marked as RESOLVED but both patches have been rejected. 
So...what is the solution for "gstreamer-0.10.36/libs/gst/base/gstadapter.c" ?
Comment 22 Tim-Philipp Müller 2013-04-25 08:12:56 UTC
Do you still get this issue using the latest GStreamer 1.0 or master branches with the above patch applied?

I stated earlier in comment #14 that I didn't understand the adapter patch and asked you to explain it in more detail - could you? (And the code probably looks a bit different in 1.x anyway)
Comment 23 Álvaro López López 2013-04-25 08:16:40 UTC
Sorry I am developing under Ubuntu Lucid Lynx 10.04, so I´m afraid I cannot build GStreamer 1.x.

However, as I have told before, my application is currently working with the "patch" written above. I have no longer experienced those segmentation faults, which brought me to start this thread.
Comment 24 Tim-Philipp Müller 2013-04-25 08:28:46 UTC
Well, I am not going to apply a patch that I don't understand and which I don't think should be needed.

Looking at the stack trace again, it seems quite likely to me that the above commit fixes the actual issue and is the correct way of fixing it.
Comment 25 Aleix Conchillo Flaqué 2013-04-26 17:47:03 UTC
I also hit the same segmentation fault and back trace.

I initially thought to do the same fix as Álvaro, but as Tim mentions I don't think this check is needed (if everything went fine before).

I think there's some issue with the JPEG depayloader when packets are lost or there's latency and jitter and that the depayloader is not very resilient to these facts.

I end up switching to TCP for JPEG streams (I send 1 image every 4 seconds).

This is the investigation that I end up with:

----------------

So here is the problematic code (see at the end). Specially:

    gst_adapter_copy (rtpjpegdepay->adapter, end, avail - 2, 2);

which will internally call:

    copy_into_unchecked (adapter, dest=end, skip=4294967294, size=2);

Note that skip is "2^32 - 2", which is the same as saying "0 - 2" for a
guint type.

My feeling is that for some reason we have an RTP packet with the marker bit
(indicating this is might be the end of the image):

  if (gst_rtp_buffer_get_marker (buf)) {

Then we get the available bytes in the adapter:

    avail = gst_adapter_available (rtpjpegdepay->adapter);

And the problem might be here, avail could be 0 which would make the following
code crash:

    gst_adapter_copy (rtpjpegdepay->adapter, end, avail - 2, 2);

So, how is it possible that the adapter doesn't contain anything if we just
did?

  gst_adapter_push (rtpjpegdepay->adapter, outbuf);

That was the only thing I could not figure out.

==============

  /* take JPEG data, push in the adapter */
  GST_DEBUG_OBJECT (rtpjpegdepay, "pushing data at offset %d", header_len);
  outbuf = gst_rtp_buffer_get_payload_subbuffer (buf, header_len, -1);
  gst_adapter_push (rtpjpegdepay->adapter, outbuf);
  outbuf = NULL;

  if (gst_rtp_buffer_get_marker (buf)) {
    guint avail;
    guint8 end[2];
    guint8 *data;

    /* last buffer take all data out of the adapter */
    avail = gst_adapter_available (rtpjpegdepay->adapter);
    GST_DEBUG_OBJECT (rtpjpegdepay, "marker set, last buffer");

    /* take the last bytes of the jpeg data to see if there is an EOI
     * marker */
    gst_adapter_copy (rtpjpegdepay->adapter, end, avail - 2, 2);

==============