Created attachment 338316 [details] [review]
Do not transmit unplayable rhythmdb entries to the DMAP client

Same trace in description, with "libdmapsharing" debug symbols installed.

+ Trace 236760

The crash is caused by an uninitialized pointer deref in libdmapsharing, so I'm reassigning the bug.  The rhythmbox patch should go in a new bug since it's not really related to the crash at all.

https://git.gnome.org/browse/libdmapsharing/tree/libdmapsharing/daap-share.c#n418
- this should probably be g_new0.

(In reply to Jonathan Matthew from comment #3)
> The crash is caused by an uninitialized pointer deref in libdmapsharing, so
> I'm reassigning the bug.  The rhythmbox patch should go in a new bug since
> it's not really related to the crash at all.

Created https://bugzilla.gnome.org/show_bug.cgi?id=773466

(In reply to Jonathan Matthew from comment #4)
> https://git.gnome.org/browse/libdmapsharing/tree/libdmapsharing/daap-share.
> c#n418
> - this should probably be g_new0.

That seems to fix the crash.

FWIW, "g_new" might need to be revisited in the following places too.

dev@unstable:~/source/apt/libdmapsharing/libdmapsharing-2.9.36$ grep "\<g_new\>" -R . 2>/dev/null
./libdmapsharing/dmap-mdns-browser-dnssd.c:	service = g_new (DMAPMdnsBrowserService, 1);
./libdmapsharing/dmap-share.c:		share_bitwise = g_new (struct share_bitwise_t, 1);
./libdmapsharing/dpap-share.c:	ChunkData *cd = g_new (ChunkData, 1);
./libdmapsharing/dmap-mdns-browser-howl.c:	service = g_new (DMAPMdnsBrowserService, 1);
./libdmapsharing/dacp-share.c:	remote_info = g_new (DACPRemoteInfo, 1);
./libdmapsharing/daap-share.c:	cd = g_new (ChunkData, 1);
./libdmapsharing/dmap-mdns-browser-avahi.c:		service = g_new (DMAPMdnsBrowserService, 1);
./libdmapsharing/dmap-mdns-publisher-avahi.c:	service = g_new (struct DMAPMdnsPublisherService, 1);
./libdmapsharing/dmap-connection.c:	rdata = g_new (ConnectionResponseData, 1);
./libdmapsharing/dmap-connection.c:	rdata = g_new (ConnectionResponseData, 1);

I decided to consistently use g_new0, even though it appeared that libdmapsharing was initializing each field in these structures after the allocation. It did appear that the structure referenced in comment #4 had each of its fields initialized too, so I am not sure the lack of g_new0 there was a problem. Can someone show otherwise?

Using g_new0 is safer in case we add a field to one of the structures in the future.

in the error case here:

        stream = G_INPUT_STREAM (daap_record_read (record, &error));
        if (error != NULL) {
                g_warning ("Couldn't open %s: %s.", location, error->message);
                goto _error;
        }

cd->stream is not initialised before we reach _error:

_error:
        soup_message_set_status (message, SOUP_STATUS_INTERNAL_SERVER_ERROR);

        if (NULL != cd) {
                if (NULL != cd->stream) {
                        g_input_stream_close (cd->stream, NULL, NULL);
                }

                g_free (cd);
        }

so it can be non-NULL and also not a valid GInputStream pointer (probably not a valid address at all), which will cause g_input_stream_close to crash.

Got it. The use of g_new0 should have fixed this. Please see 2.9.37.