fix: NPE causing segmentation fault

[ABeltramo]

Description

PulseAudio can (and will) return null when no sink is defined, assigning null to a std::string will always cause a segmentation fault.

Here's a simple check before making the assignment.

Issues Fixed or Closed

• fixes: https://github.com/SunshineStream/Sunshine/issues/226

Type of Change

• [x] Bug fix (non-breaking change which fixes an issue)

Checklist

• [x] My code follows the style guidelines of this project
• [x] I have performed a self-review of my own code
• [x] I have commented my code, particularly in hard-to-understand areas
• [x] I have added or updated the docstring/documentation-blocks for new or existing methods/components

[ReenigneArcher]

The flatpak failure is something I'm working on. It's an issue with CI.

[ReenigneArcher]

I was also thinking there is something odd about this.

https://github.com/SunshineStream/Sunshine/blob/70ae7a2fa9b33173eb949c86ec42a96e7b9e8c25/sunshine/platform/linux/audio.cpp#L391

As far as I understand this is to be defined by cmake, yet there's no build instructions I've seen that define this property.

What do you think?

[ABeltramo]

This has been introduced in 62ca9c31a07a7205c8b7d4af367a18cb2a80aff7 I took a quick look around but, unless I'm missing something, it doesn't look like it's being set anywhere. There's also @DEFAULT_MONITOR@ which looks like it's not being set.

The alternative could be that it was just used as a placeholder for when the sink or the monitor is missing so that it'll not return nullptr.

[TheElixZammuto]

This has been introduced in 62ca9c3 I took a quick look around but, unless I'm missing something, it doesn't look like it's being set anywhere. There's also @DEFAULT_MONITOR@ which looks like it's not being set.

The alternative could be that it was just used as a placeholder for when the sink or the monitor is missing so that it'll not return nullptr.

According to git blame, the change was added by #130 PR, shall we ask the owner of the PR?

[ReenigneArcher]

@Logical-sh could you give some additional insight on this?

[Logical-sh]

Those are not meant the be set by build or anything they are special fallback names.

In pulseaudio (but not for monitor in pipewire! Otherwise that pr could have been a lot simpler) those names are special device names that resolve to the default sink/monitor by the service itself.

The default string is there so if for some reason the call to get the default names fails, it attempts to use those special fallback names instead of bailing right there, instead just letting pulseaudio resolve the default devices for us.

This won't work in pipewire, as the "@DEFAULT_MONITOR@" is not implemented, and to be honest if getting the default devices fails there is likely a deeper issue anyway so its not likely to really help anything to be honest, its just similar to how the old code resolved and was a nice default value for the std:strings.

[Logical-sh]

The problem with this PR is that we use that name later to do more work. (Setting up the fake sink and redirecting streams) So while this pr might prevent a crash I am worried that this will silently eat this issue and no audio will work.

[Logical-sh]

After reading the ticket, there are a couple solutions here:

• We can fix the code to not freak out when there are no sinks, although this feels a bit odd. (Pretty quick fix, but there would be no audio even if sinks are created later)
• We could fix our code to create a dummy sink if it doesn't find a default. (Not that complicated but might take a bit more time and testing)
• I could assist the GOW on how to make a fake sink so the current code just works. (Really simple just the following commands before running sunshine

pactl load-module module-null-sink sink_name=GOWAudio
pactl update-sink-proplist GOWAudio device.description="Games on Whales Virtual Audio"
pactl set-default-sink GOWAudio

This would have the advantage of sunshine and anything ran on it having a set audio device inside the container to forward. (Not sure how your current audio solution works though!)

[ABeltramo]

Thanks @Logical-sh for the in-depth explanation, I think I understand it a little bit better now..

I think it makes sense to setup a sink on PulseAudio instead, this was likely caused by the way we run things (headless, no real audio device setup).

You seem to be knowing your way around the codebase, would you be interested in helping us out understanding and documenting it better?

[Logical-sh]

Not that I think about it, we already setup a dummy sink, we just try to route a real sink to it, and that's what is failing.

I could make a pr this weekend that fixes this null issue and lets the code still setup the dummy sink, just skipping the forwarding part.

I might not remember that 100% tho, so I'll look into it and update here later!

[ABeltramo]

Thanks, in the meantime, I'll try to add a default sink to our PulseAudio container on GOW.

Feel free to reach us on any Discord channel, I would love to chat a little bit more in depth about the codebase. I've got a few questions to ask!

[ReenigneArcher]

I'll change this to draft for now.

[github-actions[bot]]

This PR is stale because it has been open for 90 days with no activity. Comment or remove the stale label, otherwise this will be closed in 10 days.

[ReenigneArcher]

Replaced by #372