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 659573 - multipartdemux: segfault
multipartdemux: segfault
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-09-20 12:16 UTC by Nicola
Modified: 2018-11-03 14:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
file to reproduce the issue (68.66 KB, application/octet-stream)
2011-09-20 12:17 UTC, Nicola
  Details
multipartdemux: guard against having no MIME type (1.74 KB, patch)
2011-09-21 09:55 UTC, Vincent Penquerc'h
committed Details | Review
attached a patch that handle the null mime type and add two more properties (7.77 KB, patch)
2011-09-22 13:31 UTC, Nicola
reviewed Details | Review

Description Nicola 2011-09-20 12:16:42 UTC
I have the following segfault with multipartdemux:


Program received signal SIGSEGV, Segmentation fault.

Thread 140737134823168 (LWP 15906)

  • #0 g_str_hash
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #1 g_hash_table_lookup
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #2 gst_multipart_demux_get_gstname
    at multipartdemux.c line 246
  • #3 gst_multipart_find_pad_by_mime
    at multipartdemux.c line 323
  • #4 gst_multipart_demux_chain
    at multipartdemux.c line 589
  • #5 gst_pad_push
    at gstpad.c line 4710
  • #6 gst_base_src_loop
    at gstbasesrc.c line 2552
  • #7 gst_task_func
    at gsttask.c line 318
  • #8 ??
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #9 ??
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
  • #10 start_thread
    at pthread_create.c line 304
  • #11 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 112
  • #12 ??

attacched is a dump of the stream that generate the problem, you can reproduce with:

gst-launch filesrc location=... ! multipartdemux ! ...

the stream is supposed to be audio/x-raw-int, rate=8000,channels=1,endianness=1234,width=16,depth=16,signed=true
Comment 1 Nicola 2011-09-20 12:17:36 UTC
Created attachment 197043 [details]
file to reproduce the issue
Comment 2 Vincent Penquerc'h 2011-09-21 09:55:54 UTC
Created attachment 197135 [details] [review]
multipartdemux: guard against having no MIME type

The code would previously crash trying to insert a NULL string
into a hash table.
It does seem a little broken that indexing is done by MIME type
and not by index though, unless the spec says there cannot be
two parts with the same MIME type.
Comment 3 Nicola 2011-09-21 12:06:14 UTC
thanks, now exit without segfault: here is the error:

Boundary not found in the multipart header
Comment 4 Nicola 2011-09-21 12:13:35 UTC
however wireshark show this:

HTTP/1.1 200 OK
Content-Type: multipart/mixed;boundary=myboundary

--myboundary
Content-Length: 3868

so the boundary is there
Comment 5 Nicola 2011-09-21 12:26:31 UTC
what is missing is the Content-Type, for example here is a stream that multipart can handle:

HTTP/1.1 200 OK
Expires: Mon, 01 Jul 1980 00:00:00 GMT
Cache-Control: no-cache, no-store, must-revalidate
Pragma: no-cache
Content-Type: multipart/x-mixed-replace;boundary=myboundary

--myboundary
Content-Type: image/jpeg
Content-Length: 33575

... data ...

and this is the one that doens't work

HTTP/1.1 200 OK
Content-Type: multipart/mixed;boundary=myboundary

--myboundary
Content-Length: 3868

..data...

maybe multipartdemux should demux the data and forward if no content type is given
Comment 6 Nicola 2011-09-22 07:35:39 UTC
Hi Vincent,

I used another approach to handle this case. For now if mime is null I'm hardcoding the mime for my audio source if mime is null. I think it is easy enough to add a property to multipartdemux such as force-mime-if-null that allow to externally set the mime if no mime is found. If you think this property could be included in -good I'll find some time to make it, if not I'll use my modified hardcoded multipartdemux (it not break multipartdemux since I'm hardconding pad caps only if mime is null),

thanks
Nicola
Comment 7 Nicola 2011-09-22 13:31:05 UTC
Created attachment 197244 [details] [review]
attached a patch that handle the null mime type and add two more properties

This patch add the following properties:

- force-mime-if-null, allow to specify a mime if not present in the stream
- skip-first-n-bytes, remove the first n bytes from every frame, it is needed to handle some custom format

This patch allow to receive both audio and video from trendnet network cameras
Comment 8 Nicola 2011-09-28 06:24:14 UTC
Hi, I asked to trendnet if they have problems to make public available the code that allow to use their cameras in gstreamer, here is my mail:

"Hi,

I did some modifications to the gstreamer open source multimedia framework (http://gstreamer.freedesktop.org/) that make it able to receive both audio and non standard mjpeg (and I think mpeg4 too, it is not supported by my camera so I not tested it) from TV-IP121WM, do you have problems if I make this code publicy available in gstreamer (LGPL licensed)?"

here is the answer from trendnet:

"9/27/2011 2:35:52 PM
Johnny Hernandez
(Technical Support Rep)	Dear customer,

There would be no problem making code public."
Comment 9 Vincent Penquerc'h 2011-12-07 15:38:09 UTC
That does not fix the issue of indexing by MIME type, does it ?
If more than one part has the same MIME type, it won't work IIRC (not that I know if it is allowed or not by the multipart format).
Indexing by, er, index, would solve both issues (your camera not giving a MIME type, and cases with more than one part having the same MIME type).

(from memory, I haven't looked at the code again).
Comment 10 Nicola 2011-12-07 16:21:35 UTC
Hi Vincent,

I think the patch solve both the problems, using the attached test file I have the following (with the patch):

gst-launch filesrc location=/tmp/test.multipart ! multipartdemux ! decodebin2 ! pulsesink
Impostazione della pipeline a PAUSED ...
La pipeline è in PREROLLING ...
ERRORE: dall'elemento /GstPipeline:pipeline0/GstMultipartDemux:multipartdemux0: Impossibile de-multiplare lo stream.
Informazioni di debug aggiuntive:
multipartdemux.c(519): multipart_parse_header (): /GstPipeline:pipeline0/GstMultipartDemux:multipartdemux0:
Boundary not found in the multipart header
ERRORE: la pipeline non vuole fare il preroll.
Impostazione della pipeline a NULL ...
Esecuzione di free sulla pipeline...

so it give errors if no mime is found,

the patch allow to specify a mime as a property and optionally to skip the first n bytes of the buffer (it is quite common in custom format add some bytes to the begin of a frame/buffer), so if you apply the patch you can use a pipeline such this to play the attached file:

gst-launch filesrc location=/tmp/test.multipart ! multipartdemux skip-first-n-bytes=28 force-mime-if-null="audio/x-raw-int,rate=8000,channels=1,endianness=1234,width=16,depth=16,signed=true"  ! decodebin2 ! pulsesink
Comment 11 Nicola 2011-12-07 16:24:28 UTC
the relevant code in the patch is this:

     return mppad;
+  } else {
+    GST_WARNING_OBJECT (demux,
+        "No MIME type e force-mime-if-null not setted, cannot create pad");
+    if (created) {
+      *created = FALSE;
+    }
+    return NULL;
   }

I'm not using anymore the trendnet camera, I think the patch could be useful in general,

Nicola
Comment 12 Vincent Penquerc'h 2011-12-09 15:31:45 UTC
Sure, but my point was not that. It was:
If you have one part with MIME type (eg) text/plain
If you have another part with same MIME type text/plain

Then a single pad will be created, instead of one per part, because the indexing is done by a hash table keyed on the MIME type, so there is a collision.
Though maybe it was intended to be that way, I tried creating such a file, and buffers flow onto the same pad, so...
Comment 13 Nicola 2011-12-09 15:44:05 UTC
is this handled with the actual multipartdemux without my patch? Is this a real case? 

However you are right if you use force-mime-if-null property and you don't have Content-Type setted all the part without Content-Type will be forced to the value you give using force-mime-if-null
Comment 14 Vincent Penquerc'h 2011-12-09 16:00:48 UTC
It's not handled by the current code, no. That's what I was saying when I posted my original patch. Changing it to not do that (for instance, indexing each new part by their order of appearance) would fix the bug without having to add a "default" MIME type (since that MIME type would not be used as a key).

And I don't know if it's a real case, that's what I meant by "unless the spec says there cannot be two parts with the same MIME type".
Comment 15 Nicolas Dufresne (ndufresne) 2014-06-10 21:18:31 UTC
Review of attachment 197244 [details] [review]:

I'm not big fan of that approach, and can't find anything in the spec to justify. I think the input is invalid, we should not crash of course.

::: gst/multipart/multipartdemux.c
@@ +202,3 @@
+          DEFAULT_MIME_IF_NULL,
+          G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS));
+

I don't like these property names. Shorter names would do just fine. Also, rules is 80 char/lines.
Comment 16 Nicolas Dufresne (ndufresne) 2014-06-10 21:35:15 UTC
Review of attachment 197135 [details] [review]:

Minor comment, feel free to merge when/if you are happy with it. Content-type seems mandatory, so skipping or failing seems right. I didn't found anything in RFC 2046 that would allow having two streams with the same content type. This means it would have to be implement with a GStreamer extension, e.g. X-gstreamer-stream-id: <stream-id>. We could do that eventually if really needed.

::: gst/multipart/multipartdemux.c
@@ +349,3 @@
+  } else {
+    GST_WARNING_OBJECT (demux, "No MIME type, cannot create pad");
+    return NULL;

Maybe I miss something, or code have changed, but it seems like NULL isn't handled have calling gst_multipart_find_pad_by_mime(). At the same time, it will never be called with the following fix. So maybe we should put g_return_val_if_fail(), and do that at both place, as a crash avoidance, rather then handling it and crashing later ?

@@ +590,3 @@
+    } else if (G_UNLIKELY (!multipart->mime_type)) {
+      GST_DEBUG_OBJECT (multipart, "content has no MIME type.");
+      gst_adapter_flush (adapter, size - datalen);

That looks good.
Comment 17 Vincent Penquerc'h 2014-06-11 16:24:44 UTC
Yes, 7d78a68c introduced unconditional use of the pad after call of this function. Should be easy to patch too.

As for the multiple entries with the same content type, you did not find anything that allowed it, but did you find anything that forbids it ? It would be surprising if this was disallowed. After all, this is how cameras do their JPEGish streaming, lots of little parts with the same content type.
Comment 18 Tim-Philipp Müller 2014-06-11 16:45:45 UTC
> Yes, 7d78a68c introduced unconditional use of the pad after call of this
> function.

It did? As far as I can tell it adds some code that uses srcpad->foo before the already-existing gst_pad_push(srcpad->pad) call. I don't think this counts as 'introduced unconditional use'?
Comment 19 Vincent Penquerc'h 2014-06-11 16:47:21 UTC
I removed the test in the function as the test in the caller shorts it out indeed.


commit 7e278e6b22fe2eea8b620996d6f3c7bbbb30c119
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Wed Jun 11 17:43:42 2014 +0100

    multipartdemux: guard against having no MIME type
    
    The code would previously crash trying to insert a NULL string
    into a hash table.
    It does seem a little broken that indexing is done by MIME type
    and not by index though, unless the spec says there cannot be
    two parts with the same MIME type.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=659573
Comment 20 Vincent Penquerc'h 2014-06-11 16:50:35 UTC
(In reply to comment #18)
> > Yes, 7d78a68c introduced unconditional use of the pad after call of this
> > function.
> 
> It did? As far as I can tell it adds some code that uses srcpad->foo before the
> already-existing gst_pad_push(srcpad->pad) call. I don't think this counts as
> 'introduced unconditional use'?

Maybe there already was such use then ? I did not checkout that rev to look further in time. In any case, as Nicolas pointed out, that branch was not taken when there was no MIME type, so that explains why it would not have crashed with any previous dereference, so all is good (and I removed that part of my patch, since it's pointless anyway).
Comment 21 Nicolas Dufresne (ndufresne) 2014-06-11 17:25:10 UTC
(In reply to comment #17)
> As for the multiple entries with the same content type, you did not find
> anything that allowed it, but did you find anything that forbids it ? It would
> be surprising if this was disallowed. After all, this is how cameras do their
> JPEGish streaming, lots of little parts with the same content type.

I'm still trying to understand the use case you have in mind. The camera I'm dealing with have 1 stream (mime) for jpeg data, and 1 stream (mime) for audio data. Never seem any camera with multiple video streams. Note that this is all a bit of an abuse of the format, after all this was designed to attach stuff to emails, where each part is simply a different file.
Comment 22 GStreamer system administrator 2018-11-03 14:44:35 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org'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.freedesktop.org/gstreamer/gst-plugins-good/issues/49.