Bug 77595

I was the original reporter.  This is not a new problem as I originally reported it on version 0.9.21.  I've since fixed my program, but since PulseAudio is a shared resource of multiple programs it seems to me that it should be able to handle bad input like this without dying, especially since a write doesn't have to be a multiple of a sample size.

6.0 blocker.

Okay, so we're writing 64K, starting at index 1. This later fails when we're trying to drop 1 byte to get to the first package.

Why do we allow writing there at all? Looking at pa_memblockq_push, there is already a check for length, so we should probably add one for index too. There is no point in allowing an unaligned index if we don't allow unaligned lengths (right?), so the first step should be:

=============
@@ -287,7 +287,8 @@ int pa_memblockq_push(pa_memblockq* bq, const pa_memchunk *uchunk) {
     pa_assert(uchunk->length > 0);
     pa_assert(uchunk->index + uchunk->length <= pa_memblock_get_length(uchunk->memblock));
 
-    pa_assert_se(uchunk->length % bq->base == 0);
+    pa_assert(uchunk->length % bq->base == 0);
+    pa_assert(uchunk->index % bq->base == 0);
 
     if (!can_push(bq, uchunk->length))
         return -1;
=============

(This assertion has no side effect, so remove the _se suffix.)

...however, this only makes us crash slightly earlier.

So, the desired client behaviour is probably to return with an error message in case the client tries to do something we don't allow, like this:

==============
diff --git a/src/pulse/stream.c b/src/pulse/stream.c
index 8e35c29..bb94180 100644
--- a/src/pulse/stream.c
+++ b/src/pulse/stream.c
@@ -1481,6 +1481,8 @@ int pa_stream_write(
     PA_CHECK_VALIDITY(s->context, s->direction == PA_STREAM_PLAYBACK || s->direction == PA_STREAM_UPLOAD, PA_ERR_BADSTATE);
     PA_CHECK_VALIDITY(s->context, seek <= PA_SEEK_RELATIVE_END, PA_ERR_INVALID);
     PA_CHECK_VALIDITY(s->context, s->direction == PA_STREAM_PLAYBACK || (seek == PA_SEEK_RELATIVE && offset == 0), PA_ERR_INVALID);
+    PA_CHECK_VALIDITY(s->context, offset % pa_frame_size(&s->sample_spec) == 0, PA_ERR_INVALID);
+    PA_CHECK_VALIDITY(s->context, length % pa_frame_size(&s->sample_spec) == 0, PA_ERR_INVALID);
     PA_CHECK_VALIDITY(s->context,
                       !s->write_memblock ||
                       ((data >= s->write_data) &&
==============


However, that still won't stop us from malicious clients trying to crash the daemon...

David, if I understand correctly, your concern is that someone not using libpulse could emulate our protocol and cause a server crash? There are probably a few more instances where this can happen, not the least of which is writing past the SHM boundary which I believe can cause a SIGBUS.

One thing I can think of is that if we find such a situation, the assert could be converted into an "exception" that causes the sink input to be killed.

I would say that this would need to be part of a larger effort to eliminate ways in which malicious clients could kill the daemon (which would need to include a definition of our trust model, etc.).

In the mean time, I think we should unblock the release by patching both sides as you suggest. Do you want to write up patches (if you agree)?

(In reply to Arun Raghavan from comment #4)
> David, if I understand correctly, your concern is that someone not using
> libpulse could emulate our protocol and cause a server crash?

Yup.

I've sent three patches to the ML now, the third one addressing this concern as well. Waiting for review.

Patches now pushed.