[sound]: gnome-control-center crashed with SIGABRT in pa_memblock_acquire()

Bug #1058200 reported by Jeremy Bicha on 2012-09-28
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
pulseaudio (Ubuntu)
Medium
David Henningsson

Bug Description

The Sound panel has been extremely crash-prone for the past oh, maybe 2 weeks.

ProblemType: Crash
DistroRelease: Ubuntu 12.10
Package: gnome-control-center 1:3.4.2-0ubuntu15
ProcVersionSignature: Ubuntu 3.5.0-15.23-generic 3.5.4
Uname: Linux 3.5.0-15-generic x86_64
ApportVersion: 2.5.2-0ubuntu4
Architecture: amd64
CrashCounter: 1
Date: Wed Sep 26 15:34:20 2012
ExecutablePath: /usr/bin/gnome-control-center
ProcCmdline: gnome-control-center --overview
Signal: 6
SourcePackage: gnome-control-center
StacktraceTop:
 raise () from /lib/x86_64-linux-gnu/libc.so.6
 abort () from /lib/x86_64-linux-gnu/libc.so.6
 pa_memblock_acquire () from /usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-2.1.so
 pa_stream_peek () from /usr/lib/x86_64-linux-gnu/libpulse.so.0
 ?? () from /usr/lib/control-center-1/panels/libsound.so
Title: [sound]: gnome-control-center crashed with SIGABRT in raise()
UpgradeStatus: No upgrade log present (probably fresh install)
UserGroups: adm cdrom dip libvirtd lpadmin plugdev sambashare sbuild sudo
usr_lib_gnome-control-center:
 activity-log-manager-control-center 0.9.4-0ubuntu3
 deja-dup 23.92-0ubuntu1
 gnome-control-center-signon 0.0.17-0ubuntu1
 indicator-datetime 12.10.1-0ubuntu1

Jeremy Bicha (jbicha) wrote :

StacktraceTop:
 pa_memblock_acquire (b=<optimized out>) at pulsecore/memblock.c:454
 pa_memblock_acquire (b=<optimized out>) at pulsecore/memblock.c:453
 pa_stream_peek (s=s@entry=0x7f6b582eaca0, data=data@entry=0x7fff951e3b48, length=length@entry=0x7fff951e3b38) at pulse/stream.c:1602
 on_monitor_read_callback (s=0x7f6b582eaca0, length=4, userdata=0x7f6b5823c020) at gvc-mixer-dialog.c:474
 pstream_memblock_callback (p=<optimized out>, channel=<optimized out>, offset=0, seek=PA_SEEK_RELATIVE, chunk=0x7fff951e3c00, userdata=0x7f6b58200cb0) at pulse/context.c:365

Changed in gnome-control-center (Ubuntu):
importance: Undecided → Medium
summary: - [sound]: gnome-control-center crashed with SIGABRT in raise()
+ [sound]: gnome-control-center crashed with SIGABRT in
+ pa_memblock_acquire()
tags: removed: need-amd64-retrace
Sebastien Bacher (seb128) wrote :

Hum, that stacktrace points to pulseaudio, and the only update which matches that timeframe is:
https://launchpad.net/ubuntu/+source/pulseaudio/1:2.1-0ubuntu3

could you try if the issue still happen with the previous version?

visibility: private → public
affects: gnome-control-center (Ubuntu) → pulseaudio (Ubuntu)

If there is no silence memblock and no data, pa_memblockq_peek can
return NULL. In this case, do not crash on an assertion in
pa_memblock_acquire, but instead return a proper error to the client.

BugLink: http://bugs.launchpad.net/bugs/1058200
Signed-off-by: David Henningsson <email address hidden>
---
 src/pulse/stream.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/pulse/stream.c b/src/pulse/stream.c
index 2b6d306..9bb0995 100644
--- a/src/pulse/stream.c
+++ b/src/pulse/stream.c
@@ -1598,6 +1598,8 @@ int pa_stream_peek(pa_stream *s, const void **data, size_t *length) {
             return 0;
         }

+ PA_CHECK_VALIDITY(s->context, s->peek_memchunk.memblock != NULL, PA_ERR_NODATA);
+
         s->peek_data = pa_memblock_acquire(s->peek_memchunk.memblock);
     }

--
1.7.9.5

Changed in pulseaudio (Ubuntu):
status: New → In Progress
assignee: nobody → David Henningsson (diwic)

On Mon, 2012-10-01 at 17:06 +0200, David Henningsson wrote:
> If there is no silence memblock and no data, pa_memblockq_peek can
> return NULL. In this case, do not crash on an assertion in
> pa_memblock_acquire, but instead return a proper error to the client.

If there is no data in the buffer, pa_stream_peek() is supposed to
return NULL according to the documentation. And it does that: if there's
no data, pa_memblock_peek() will return a negative value, causing
pa_stream_peek() to return NULL.

The problem is the case where the buffer does contain data, but not at
the read index. That is, there is a hole in the buffer. The client
documentation doesn't have any warnings about holes, so the only safe
way to handle holes is to return silence. Fixing this should be a simple
matter of giving a silence memchunk when creating record_memblockq.

--
Tanu

David Henningsson (diwic) wrote :

On 10/02/2012 10:38 PM, Tanu Kaskinen wrote:
> On Mon, 2012-10-01 at 17:06 +0200, David Henningsson wrote:
>> If there is no silence memblock and no data, pa_memblockq_peek can
>> return NULL. In this case, do not crash on an assertion in
>> pa_memblock_acquire, but instead return a proper error to the client.
>
> If there is no data in the buffer, pa_stream_peek() is supposed to
> return NULL according to the documentation. And it does that: if there's
> no data, pa_memblock_peek() will return a negative value, causing
> pa_stream_peek() to return NULL.
>
> The problem is the case where the buffer does contain data, but not at
> the read index. That is, there is a hole in the buffer. The client
> documentation doesn't have any warnings about holes, so the only safe
> way to handle holes is to return silence. Fixing this should be a simple
> matter of giving a silence memchunk when creating record_memblockq.

I'm not so sure. Silence, as in all zeroes, might work for S16 audio
data, but what about other formats? Compressed audio? Peak audio (which
I think is the case here)? Etc.

Also maybe it could also be valuable for the client to distinguish
between no data available, and valid zero data.

How about returning NULL and adding to the documentation something like:

-If no data is available this will return a NULL pointer.
+If no data is available (at the current read position), this will
return a NULL pointer.

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

If there is no silence memblock and no data, pa_memblockq_peek can
return NULL. In this case, do not crash on an assertion in
pa_memblock_acquire, but instead return NULL.

BugLink: http://bugs.launchpad.net/bugs/1058200
Signed-off-by: David Henningsson <email address hidden>
---
 src/pulse/stream.c | 3 ++-
 src/pulse/stream.h | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/pulse/stream.c b/src/pulse/stream.c
index 2b6d306..0defb4f 100644
--- a/src/pulse/stream.c
+++ b/src/pulse/stream.c
@@ -1592,7 +1592,8 @@ int pa_stream_peek(pa_stream *s, const void **data, size_t *length) {

     if (!s->peek_memchunk.memblock) {

- if (pa_memblockq_peek(s->record_memblockq, &s->peek_memchunk) < 0) {
+ if (pa_memblockq_peek(s->record_memblockq, &s->peek_memchunk) < 0 ||
+ !s->peek_memchunk.memblock) {
             *data = NULL;
             *length = 0;
             return 0;
diff --git a/src/pulse/stream.h b/src/pulse/stream.h
index b4464fa..8665d13 100644
--- a/src/pulse/stream.h
+++ b/src/pulse/stream.h
@@ -537,8 +537,8 @@ int pa_stream_write(
  * \a data will point to the actual data and \a nbytes will contain the size
  * of the data in bytes (which can be less or more than a complete
  * fragment). Use pa_stream_drop() to actually remove the data from
- * the buffer. If no data is available this will return a NULL
- * pointer. */
+ * the buffer. If no data is available (at the current read position)
+ * this will return a NULL pointer. */
 int pa_stream_peek(
         pa_stream *p /**< The stream to use */,
         const void **data /**< Pointer to pointer that will point to data */,
--
1.7.9.5

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package pulseaudio - 1:2.1-0ubuntu4

---------------
pulseaudio (1:2.1-0ubuntu4) quantal-proposed; urgency=low

  * 0101-alsa-mixer-Remove-analog-output-lfe-on-mono.patch:
    Fix muted audio on startup in Virtualbox VM (LP: #1016969)
  * 0020-stream-Return-error-in-case-a-client-peeks-to-early.patch:
    Fix clients crashing when asking for data, but there is no data
    to hand out (LP: #1058200)
 -- David Henningsson <email address hidden> Thu, 04 Oct 2012 14:43:27 +0200

Changed in pulseaudio (Ubuntu):
status: In Progress → Fix Released

On Wed, 2012-10-03 at 08:50 +0200, David Henningsson wrote:
> On 10/02/2012 10:38 PM, Tanu Kaskinen wrote:
> > On Mon, 2012-10-01 at 17:06 +0200, David Henningsson wrote:
> >> If there is no silence memblock and no data, pa_memblockq_peek can
> >> return NULL. In this case, do not crash on an assertion in
> >> pa_memblock_acquire, but instead return a proper error to the client.
> >
> > If there is no data in the buffer, pa_stream_peek() is supposed to
> > return NULL according to the documentation. And it does that: if there's
> > no data, pa_memblock_peek() will return a negative value, causing
> > pa_stream_peek() to return NULL.
> >
> > The problem is the case where the buffer does contain data, but not at
> > the read index. That is, there is a hole in the buffer. The client
> > documentation doesn't have any warnings about holes, so the only safe
> > way to handle holes is to return silence. Fixing this should be a simple
> > matter of giving a silence memchunk when creating record_memblockq.
>
> I'm not so sure. Silence, as in all zeroes, might work for S16 audio
> data, but what about other formats? Compressed audio? Peak audio (which
> I think is the case here)? Etc.

Good point. Regarding PCM, if pa_memchunk_silence() is used, the
function will take care of filling the memory with appropriate content.
But that doesn't work with compressed audio.

> Also maybe it could also be valuable for the client to distinguish
> between no data available, and valid zero data.
>
> How about returning NULL and adding to the documentation something like:
>
> -If no data is available this will return a NULL pointer.
> +If no data is available (at the current read position), this will
> return a NULL pointer.

An addition: the client probably wants to know how large the hole is. It
might be possible to figure that out somehow from the read index, but I
think it would make sense to return the hole size in the length
parameter.

--
Tanu

2012-11-03 17:19, Colin Guthrie skrev:
> 'Twas brillig, and Tanu Kaskinen at 05/10/12 13:58 did gyre and gimble:
>> On Wed, 2012-10-03 at 08:50 +0200, David Henningsson wrote:
>>> On 10/02/2012 10:38 PM, Tanu Kaskinen wrote:
>>>> On Mon, 2012-10-01 at 17:06 +0200, David Henningsson wrote:
>>>>> If there is no silence memblock and no data, pa_memblockq_peek can
>>>>> return NULL. In this case, do not crash on an assertion in
>>>>> pa_memblock_acquire, but instead return a proper error to the client.
>>>> If there is no data in the buffer, pa_stream_peek() is supposed to
>>>> return NULL according to the documentation. And it does that: if there's
>>>> no data, pa_memblock_peek() will return a negative value, causing
>>>> pa_stream_peek() to return NULL.
>>>>
>>>> The problem is the case where the buffer does contain data, but not at
>>>> the read index. That is, there is a hole in the buffer. The client
>>>> documentation doesn't have any warnings about holes, so the only safe
>>>> way to handle holes is to return silence. Fixing this should be a simple
>>>> matter of giving a silence memchunk when creating record_memblockq.
>>> I'm not so sure. Silence, as in all zeroes, might work for S16 audio
>>> data, but what about other formats? Compressed audio? Peak audio (which
>>> I think is the case here)? Etc.
>> Good point. Regarding PCM, if pa_memchunk_silence() is used, the
>> function will take care of filling the memory with appropriate content.
>> But that doesn't work with compressed audio.
>>
>>> Also maybe it could also be valuable for the client to distinguish
>>> between no data available, and valid zero data.
>>>
>>> How about returning NULL and adding to the documentation something like:
>>>
>>> -If no data is available this will return a NULL pointer.
>>> +If no data is available (at the current read position), this will
>>> return a NULL pointer.
>> An addition: the client probably wants to know how large the hole is. It
>> might be possible to figure that out somehow from the read index, but I
>> think it would make sense to return the hole size in the length
>> parameter.
> This discussion seemed to stagnate. Is this worth fixing/documenting for
> the 3.0 release?
>
> Col
>
>
Returning NULL seems to be the right thing to do here, even if
gnome-control-center does not handle that very well IIRC. So we might
need an additional patch in g-c-c.
So assuming I commit a patch doing that. If somebody else wants to add
logic to figure out how large the hole is, that could be discussed
separately.
Any objections?

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

On Sat, 2012-11-03 at 19:51 +0100, David Henningsson wrote:
> 2012-11-03 17:19, Colin Guthrie skrev:
> > 'Twas brillig, and Tanu Kaskinen at 05/10/12 13:58 did gyre and gimble:
> >> On Wed, 2012-10-03 at 08:50 +0200, David Henningsson wrote:
> >>> On 10/02/2012 10:38 PM, Tanu Kaskinen wrote:
> >>>> On Mon, 2012-10-01 at 17:06 +0200, David Henningsson wrote:
> >>>>> If there is no silence memblock and no data, pa_memblockq_peek can
> >>>>> return NULL. In this case, do not crash on an assertion in
> >>>>> pa_memblock_acquire, but instead return a proper error to the client.
> >>>> If there is no data in the buffer, pa_stream_peek() is supposed to
> >>>> return NULL according to the documentation. And it does that: if there's
> >>>> no data, pa_memblock_peek() will return a negative value, causing
> >>>> pa_stream_peek() to return NULL.
> >>>>
> >>>> The problem is the case where the buffer does contain data, but not at
> >>>> the read index. That is, there is a hole in the buffer. The client
> >>>> documentation doesn't have any warnings about holes, so the only safe
> >>>> way to handle holes is to return silence. Fixing this should be a simple
> >>>> matter of giving a silence memchunk when creating record_memblockq.
> >>> I'm not so sure. Silence, as in all zeroes, might work for S16 audio
> >>> data, but what about other formats? Compressed audio? Peak audio (which
> >>> I think is the case here)? Etc.
> >> Good point. Regarding PCM, if pa_memchunk_silence() is used, the
> >> function will take care of filling the memory with appropriate content.
> >> But that doesn't work with compressed audio.
> >>
> >>> Also maybe it could also be valuable for the client to distinguish
> >>> between no data available, and valid zero data.
> >>>
> >>> How about returning NULL and adding to the documentation something like:
> >>>
> >>> -If no data is available this will return a NULL pointer.
> >>> +If no data is available (at the current read position), this will
> >>> return a NULL pointer.
> >> An addition: the client probably wants to know how large the hole is. It
> >> might be possible to figure that out somehow from the read index, but I
> >> think it would make sense to return the hole size in the length
> >> parameter.
> > This discussion seemed to stagnate. Is this worth fixing/documenting for
> > the 3.0 release?
> >
> > Col
> >
> >
> Returning NULL seems to be the right thing to do here, even if
> gnome-control-center does not handle that very well IIRC. So we might
> need an additional patch in g-c-c.
> So assuming I commit a patch doing that. If somebody else wants to add
> logic to figure out how large the hole is, that could be discussed
> separately.
> Any objections?

It's not clear what you meant by "add logic to figure out how large the
hole is". Add to where? pa_stream_peek() or gnome-control-center?

To me, reporting the hole length in the "nbytes" parameter of
pa_stream_peek() seems like the right thing to do, so I hope your patch
will do this.

--
Tanu

David Henningsson (diwic) wrote :
Download full text (3.2 KiB)

On 11/04/2012 02:22 PM, Tanu Kaskinen wrote:
> On Sat, 2012-11-03 at 19:51 +0100, David Henningsson wrote:
>> 2012-11-03 17:19, Colin Guthrie skrev:
>>> 'Twas brillig, and Tanu Kaskinen at 05/10/12 13:58 did gyre and gimble:
>>>> On Wed, 2012-10-03 at 08:50 +0200, David Henningsson wrote:
>>>>> On 10/02/2012 10:38 PM, Tanu Kaskinen wrote:
>>>>>> On Mon, 2012-10-01 at 17:06 +0200, David Henningsson wrote:
>>>>>>> If there is no silence memblock and no data, pa_memblockq_peek can
>>>>>>> return NULL. In this case, do not crash on an assertion in
>>>>>>> pa_memblock_acquire, but instead return a proper error to the client.
>>>>>> If there is no data in the buffer, pa_stream_peek() is supposed to
>>>>>> return NULL according to the documentation. And it does that: if there's
>>>>>> no data, pa_memblock_peek() will return a negative value, causing
>>>>>> pa_stream_peek() to return NULL.
>>>>>>
>>>>>> The problem is the case where the buffer does contain data, but not at
>>>>>> the read index. That is, there is a hole in the buffer. The client
>>>>>> documentation doesn't have any warnings about holes, so the only safe
>>>>>> way to handle holes is to return silence. Fixing this should be a simple
>>>>>> matter of giving a silence memchunk when creating record_memblockq.
>>>>> I'm not so sure. Silence, as in all zeroes, might work for S16 audio
>>>>> data, but what about other formats? Compressed audio? Peak audio (which
>>>>> I think is the case here)? Etc.
>>>> Good point. Regarding PCM, if pa_memchunk_silence() is used, the
>>>> function will take care of filling the memory with appropriate content.
>>>> But that doesn't work with compressed audio.
>>>>
>>>>> Also maybe it could also be valuable for the client to distinguish
>>>>> between no data available, and valid zero data.
>>>>>
>>>>> How about returning NULL and adding to the documentation something like:
>>>>>
>>>>> -If no data is available this will return a NULL pointer.
>>>>> +If no data is available (at the current read position), this will
>>>>> return a NULL pointer.
>>>> An addition: the client probably wants to know how large the hole is. It
>>>> might be possible to figure that out somehow from the read index, but I
>>>> think it would make sense to return the hole size in the length
>>>> parameter.
>>> This discussion seemed to stagnate. Is this worth fixing/documenting for
>>> the 3.0 release?
>>>
>> Returning NULL seems to be the right thing to do here, even if
>> gnome-control-center does not handle that very well IIRC. So we might
>> need an additional patch in g-c-c.
>> So assuming I commit a patch doing that. If somebody else wants to add
>> logic to figure out how large the hole is, that could be discussed
>> separately.
>> Any objections?
>
> It's not clear what you meant by "add logic to figure out how large the
> hole is". Add to where? pa_stream_peek() or gnome-control-center?

I was referring to your earlier comment "An addition: the client
probably wants to know how large the hole is.", i e pa_stream_peek.

> To me, reporting the hole length in the "nbytes" parameter of
> pa_stream_peek() seems like the right thing to do, so I hope your patch
> will do this.

It do...

Read more...

Tanu Kaskinen (tanuk) wrote :

On Sun, 2012-11-04 at 23:09 +0100, David Henningsson wrote:
> On 11/04/2012 02:22 PM, Tanu Kaskinen wrote:
> > On Sat, 2012-11-03 at 19:51 +0100, David Henningsson wrote:
> >> Returning NULL seems to be the right thing to do here, even if
> >> gnome-control-center does not handle that very well IIRC. So we might
> >> need an additional patch in g-c-c.
> >> So assuming I commit a patch doing that. If somebody else wants to add
> >> logic to figure out how large the hole is, that could be discussed
> >> separately.
> >> Any objections?
> >
> > It's not clear what you meant by "add logic to figure out how large the
> > hole is". Add to where? pa_stream_peek() or gnome-control-center?
>
> I was referring to your earlier comment "An addition: the client
> probably wants to know how large the hole is.", i e pa_stream_peek.
>
> > To me, reporting the hole length in the "nbytes" parameter of
> > pa_stream_peek() seems like the right thing to do, so I hope your patch
> > will do this.
>
> It does not: I was just suggesting to discuss that separately.

Ok. Well, I'm suggesting to not make these issues separate, since it's a
small thing to decide and implement, and doing a separate fix would be
"fixing a fix". Does someone have an issue with returning the hole size
in the nbytes argument?

--
Tanu

David Henningsson (diwic) wrote :

On 11/05/2012 07:13 PM, Tanu Kaskinen wrote:
> On Sun, 2012-11-04 at 23:09 +0100, David Henningsson wrote:
>> On 11/04/2012 02:22 PM, Tanu Kaskinen wrote:
>>> On Sat, 2012-11-03 at 19:51 +0100, David Henningsson wrote:
>>>> Returning NULL seems to be the right thing to do here, even if
>>>> gnome-control-center does not handle that very well IIRC. So we might
>>>> need an additional patch in g-c-c.
>>>> So assuming I commit a patch doing that. If somebody else wants to add
>>>> logic to figure out how large the hole is, that could be discussed
>>>> separately.
>>>> Any objections?
>>>
>>> It's not clear what you meant by "add logic to figure out how large the
>>> hole is". Add to where? pa_stream_peek() or gnome-control-center?
>>
>> I was referring to your earlier comment "An addition: the client
>> probably wants to know how large the hole is.", i e pa_stream_peek.
>>
>>> To me, reporting the hole length in the "nbytes" parameter of
>>> pa_stream_peek() seems like the right thing to do, so I hope your patch
>>> will do this.
>>
>> It does not: I was just suggesting to discuss that separately.
>
> Ok. Well, I'm suggesting to not make these issues separate, since it's a
> small thing to decide and implement, and doing a separate fix would be
> "fixing a fix".

I don't have an issue with step-by-step improvements. I see this as such
(if the second step is to be seen as an improvement).

> Does someone have an issue with returning the hole size
> in the nbytes argument?

Not if you're volunteering to do the work. All I care about is that PA
clients can crash for this reason, and I'd like that to stop happening.

Note: gnome-control-center needs a corresponding fix either way, as it
currently checks the length rather than the NULL pointer, IIRC.

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Tanu Kaskinen (tanuk) wrote :

On Tue, 2012-11-06 at 08:36 +0100, David Henningsson wrote:
> On 11/05/2012 07:13 PM, Tanu Kaskinen wrote:
> > On Sun, 2012-11-04 at 23:09 +0100, David Henningsson wrote:
> >> On 11/04/2012 02:22 PM, Tanu Kaskinen wrote:
> >>> On Sat, 2012-11-03 at 19:51 +0100, David Henningsson wrote:
> >>>> Returning NULL seems to be the right thing to do here, even if
> >>>> gnome-control-center does not handle that very well IIRC. So we might
> >>>> need an additional patch in g-c-c.
> >>>> So assuming I commit a patch doing that. If somebody else wants to add
> >>>> logic to figure out how large the hole is, that could be discussed
> >>>> separately.
> >>>> Any objections?
> >>>
> >>> It's not clear what you meant by "add logic to figure out how large the
> >>> hole is". Add to where? pa_stream_peek() or gnome-control-center?
> >>
> >> I was referring to your earlier comment "An addition: the client
> >> probably wants to know how large the hole is.", i e pa_stream_peek.
> >>
> >>> To me, reporting the hole length in the "nbytes" parameter of
> >>> pa_stream_peek() seems like the right thing to do, so I hope your patch
> >>> will do this.
> >>
> >> It does not: I was just suggesting to discuss that separately.
> >
> > Ok. Well, I'm suggesting to not make these issues separate, since it's a
> > small thing to decide and implement, and doing a separate fix would be
> > "fixing a fix".
>
> I don't have an issue with step-by-step improvements. I see this as such
> (if the second step is to be seen as an improvement).
>
> > Does someone have an issue with returning the hole size
> > in the nbytes argument?
>
> Not if you're volunteering to do the work. All I care about is that PA
> clients can crash for this reason, and I'd like that to stop happening.

Cool, I certainly volunteer to do the work. It should be trivial, so
I'll probably send a patch today.

> Note: gnome-control-center needs a corresponding fix either way, as it
> currently checks the length rather than the NULL pointer, IIRC.

Mmm, I guess I should take this task too.

--
Tanu

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers