PulseAudio Managers

[Paul Smith]

On Tue, 2009-10-20 at 00:39 -0400, Daniel Chen wrote:
> On Tue, Oct 20, 2009 at 12:14 AM, Paul Smith <paul at mad-scientist.us> wrote:
> > time, but this change looks like a dud to me.
> 
> Clearly the right way to debug this is to comment out the patch in
> debian/series and see why pa_streq() is being passed crap. Anyone with
> bt hardware and valgrind up for it?
> 
> http://launchpadlibrarian.net/32493850/Stacktrace.txt

The question is what's the point of the patch at all?  If the contents
of address are bad, what's gained by allocating memory, copying the bad
contents into it, and comparing that (then freeing the memory)?

Either the address pointer itself is pointing into bad memory, in which
case the act of sprintf'ing it into the newly allocated memory will fail
just like the strcmp does, or else the string that address points to is
"bad" in some way, in which case what's the point of making a copy of
it; the copy will just be bad the same way anyway.

Plus, unless pa_sprintf_malloc() can handle NULL pointers properly
(possible, I didn't check) this patch actually INTRODUCES a bug that
wasn't present in the original code, by not testing address for NULL
before using it.

The patch should be reverted and the bug should be reopened: this change
has no chance of solving the problem.


Looking at the stacktrace above, it seems that address is a perfectly
legal pointer, pointing to the string "(null)" (not a NULL pointer!)
Maybe there was some confusion about this.  It seems that this string
was generated in the pa_hook_fire() function.  Maybe someone passed a
NULL pointer to a sprintf() variant here; on some systems (Solaris for
example) if you pass a NULL pointer to *printf() for a %s character,
rather than dumping core, it just prints a token like "(null)".
Personally I think this is a stupid idea; I'd much rather get a core
dump I can debug than have random data generated by my code with no
errors.

Since address is known to not be NULL, that must mean d->address is NULL
since that's apparently why the core dump was generated.  Since the
prior if-statement seems to ensure that d is not NULL, that means that
d->address itself is NULL.  I have no idea how/why that might happen.

If you want a change that will inhibit the core dump, changing the test
from:

        if (address && !(pa_streq(d->address, address))) {

to:

        if (address && d->address & !(pa_streq(d->address, address))) {

would help a lot more than the current change, as far as I can see.

Cheers!