Fixing null free() causing assertion failures with pulse (#202)

[alexmikoto]

Looks like the pulse client isOpen logic is flipped, causing all sorts of weird bugs? Not completely familiar with the internals of this project so feedback appreciated.

[Copilot]

Pull request overview

Fixes PulseAudio source open/closed state detection so the rest of the audio pipeline can correctly gate reads/closes and avoid invalid PulseAudio handle usage.

Changes:

• Correct PulseAudioDspSource::isOpen() to report open when the PulseAudio handle is non-null.
• Reinstate an isOpen() guard in canRead() so it returns false when the source is not open.
• Null out the PulseAudio handle after pa_simple_free() to prevent reuse/double-free.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
darkice/src/PulseAudioDspSource.h	Fixes isOpen() semantics (in both duplicated header blocks).
darkice/src/PulseAudioDspSource.cpp	Adds isOpen() guard in canRead() and clears s after pa_simple_free().

Comments suppressed due to low confidence (1)

darkice/src/PulseAudioDspSource.cpp:198

• With the new early-return if ( !isOpen() ) return false;, the subsequent if (running == 0) { ... pa_simple_new ... } block can no longer run in normal operation (when running == 0, s should be NULL and isOpen() will already have returned false). Consider removing the now-dead lazy-open block, or (if lazy-open is desired) dropping the early-return and basing the logic solely on running/s in a consistent way.

    if (running == 0) {
      char         client_name[255];
      
      //to identify each darkice on pulseaudio server
      snprintf(client_name, 255, "darkice-%d", getpid());
      
      if (!(s = pa_simple_new(NULL, client_name, PA_STREAM_RECORD, sourceName, "darkice record", &ss, NULL, NULL, &error))) {
        throw Exception( __FILE__, __LINE__, ": pa_simple_new() failed: %s\n", pa_strerror(error));
      }
      
      running = 1;
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

canRead() now calls isOpen() immediately; however isOpen() is based on the pa_simple* s member, and s is never initialized in init()/constructors. That means isOpen() may read an indeterminate pointer value before open() is called (e.g., in canRead(), strip(), copy/assignment), leading to undefined behavior and potentially calling pa_simple_free() on a garbage pointer. Initialize s to NULL in init() (and ensure strip()/close() leave it NULL) so isOpen() is safe before the first open().

[rafael2k]

@copilot open a new pull request to apply changes based on this feedback

[Copilot]

This header appears to contain two copies of the PulseAudioDspSource definition separated by an #endif /* PULSEAUDIO_SOURCE_H */ and then a second #ifndef PULSEAUDIO_SOURCE_H block. Because the include guard macro is already defined, the second half is effectively dead code, but it still has to be kept in sync (as shown by needing to change isOpen() twice). Consider removing the duplicated second copy and keeping a single guarded definition to prevent future divergence/confusion.

[rafael2k]

@copilot open a new pull request to apply changes based on this feedback