From 243011896ad2503e515b4fed746402e651b8e520 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volker=20R=C3=BCmelin?= Date: Mon, 17 May 2021 21:46:01 +0200 Subject: [PATCH 1/7] alsaaudio: remove #ifdef DEBUG to avoid bit rot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge the #ifdef DEBUG code with the if statement a few lines above to avoid bit rot. Suggested-by: Gerd Hoffmann Signed-off-by: Volker Rümelin Message-Id: <20210517194604.2545-1-vr_qemu@t-online.de> Signed-off-by: Gerd Hoffmann --- audio/alsaaudio.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c index fcc2f62864fb..2b9789e64771 100644 --- a/audio/alsaaudio.c +++ b/audio/alsaaudio.c @@ -34,6 +34,8 @@ #define AUDIO_CAP "alsa" #include "audio_int.h" +#define DEBUG_ALSA 0 + struct pollhlp { snd_pcm_t *handle; struct pollfd *pfds; @@ -587,16 +589,12 @@ static int alsa_open(bool in, struct alsa_params_req *req, *handlep = handle; - if (obtfmt != req->fmt || - obt->nchannels != req->nchannels || - obt->freq != req->freq) { + if (DEBUG_ALSA || obtfmt != req->fmt || + obt->nchannels != req->nchannels || obt->freq != req->freq) { dolog ("Audio parameters for %s\n", typ); alsa_dump_info(req, obt, obtfmt, apdo); } -#ifdef DEBUG - alsa_dump_info(req, obt, obtfmt, apdo); -#endif return 0; err: From 50db82d84ce24e893932ecb1aa90cc9c5560fc91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volker=20R=C3=BCmelin?= Date: Mon, 17 May 2021 21:46:02 +0200 Subject: [PATCH 2/7] paaudio: remove unused stream flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In current code there are no calls to pa_stream_get_latency() or pa_stream_get_time() to receive latency or time information. Remove the flags PA_STREAM_INTERPOLATE_TIMING and PA_STREAM_AUTO_TIMING_UPDATE which instruct PulseAudio to calculate this information in regular intervals. Signed-off-by: Volker Rümelin Message-Id: <20210517194604.2545-2-vr_qemu@t-online.de> Signed-off-by: Gerd Hoffmann --- audio/paaudio.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/audio/paaudio.c b/audio/paaudio.c index c97b22e970d8..14b4269c5500 100644 --- a/audio/paaudio.c +++ b/audio/paaudio.c @@ -463,10 +463,7 @@ static pa_stream *qpa_simple_new ( pa_stream_set_state_callback(stream, stream_state_cb, c); - flags = - PA_STREAM_INTERPOLATE_TIMING - | PA_STREAM_AUTO_TIMING_UPDATE - | PA_STREAM_EARLY_REQUESTS; + flags = PA_STREAM_EARLY_REQUESTS; if (dev) { /* don't move the stream if the user specified a sink/source */ From 37a54d054f5aac43cb5721c68954b8b76d0db12d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volker=20R=C3=BCmelin?= Date: Mon, 17 May 2021 21:46:03 +0200 Subject: [PATCH 3/7] audio: move code to audio/audio.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the code to generate the pa_context_new() application name argument to a function in audio/audio.c. The new function audio_application_name() will also be used in the jackaudio backend. Signed-off-by: Volker Rümelin Message-Id: <20210517194604.2545-3-vr_qemu@t-online.de> Signed-off-by: Gerd Hoffmann --- audio/audio.c | 9 +++++++++ audio/audio_int.h | 2 ++ audio/paaudio.c | 5 +---- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/audio/audio.c b/audio/audio.c index 534278edfed2..052ca6cb789b 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -32,6 +32,7 @@ #include "qapi/qapi-visit-audio.h" #include "qemu/cutils.h" #include "qemu/module.h" +#include "qemu-common.h" #include "sysemu/replay.h" #include "sysemu/runstate.h" #include "ui/qemu-spice.h" @@ -2172,6 +2173,14 @@ const char *audio_get_id(QEMUSoundCard *card) } } +const char *audio_application_name(void) +{ + const char *vm_name; + + vm_name = qemu_get_vm_name(); + return vm_name ? vm_name : "qemu"; +} + void audio_rate_start(RateCtl *rate) { memset(rate, 0, sizeof(RateCtl)); diff --git a/audio/audio_int.h b/audio/audio_int.h index 06f0913835b0..6d685e24a388 100644 --- a/audio/audio_int.h +++ b/audio/audio_int.h @@ -243,6 +243,8 @@ void *audio_calloc (const char *funcname, int nmemb, size_t size); void audio_run(AudioState *s, const char *msg); +const char *audio_application_name(void); + typedef struct RateCtl { int64_t start_ticks; int64_t bytes_sent; diff --git a/audio/paaudio.c b/audio/paaudio.c index 14b4269c5500..75401d53910a 100644 --- a/audio/paaudio.c +++ b/audio/paaudio.c @@ -2,7 +2,6 @@ #include "qemu/osdep.h" #include "qemu/module.h" -#include "qemu-common.h" #include "audio.h" #include "qapi/opts-visitor.h" @@ -753,7 +752,6 @@ static int qpa_validate_per_direction_opts(Audiodev *dev, /* common */ static void *qpa_conn_init(const char *server) { - const char *vm_name; PAConnection *c = g_malloc0(sizeof(PAConnection)); QTAILQ_INSERT_TAIL(&pa_conns, c, list); @@ -762,9 +760,8 @@ static void *qpa_conn_init(const char *server) goto fail; } - vm_name = qemu_get_vm_name(); c->context = pa_context_new(pa_threaded_mainloop_get_api(c->mainloop), - vm_name ? vm_name : "qemu"); + audio_application_name()); if (!c->context) { goto fail; } From 2833d697b9a418e2b9735e38ad4b33ae86f84739 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volker=20R=C3=BCmelin?= Date: Mon, 17 May 2021 21:46:04 +0200 Subject: [PATCH 4/7] jackaudio: avoid that the client name contains the word (NULL) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently with jackaudio client name and qemu guest name unset, the JACK client names are out-(NULL) and in-(NULL). These names are user visible in the patch bay. Replace the function call to qemu_get_vm_name() with a call to audio_application_name() which replaces NULL with "qemu" to have more descriptive names. Signed-off-by: Volker Rümelin Message-Id: <20210517194604.2545-4-vr_qemu@t-online.de> Signed-off-by: Gerd Hoffmann --- audio/jackaudio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/audio/jackaudio.c b/audio/jackaudio.c index 3031c4e29bd4..e7de6d5433e9 100644 --- a/audio/jackaudio.c +++ b/audio/jackaudio.c @@ -26,7 +26,6 @@ #include "qemu/module.h" #include "qemu/atomic.h" #include "qemu/main-loop.h" -#include "qemu-common.h" #include "audio.h" #define AUDIO_CAP "jack" @@ -412,7 +411,7 @@ static int qjack_client_init(QJackClient *c) snprintf(client_name, sizeof(client_name), "%s-%s", c->out ? "out" : "in", - c->opt->client_name ? c->opt->client_name : qemu_get_vm_name()); + c->opt->client_name ? c->opt->client_name : audio_application_name()); if (c->opt->exact_name) { options |= JackUseExactName; From a2cd86a94a881b38a7d8bb67c61920ab3b23e82d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Wed, 16 Jun 2021 12:43:49 +0200 Subject: [PATCH 5/7] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While the SB16 seems to work up to 48000 Hz, the "Sound Blaster Series Hardware Programming Guide" limit the sampling range from 4000 Hz to 44100 Hz (Section 3-9, 3-10: Digitized Sound I/O Programming, tables 3-2 and 3-3). Later, section 6-15 (DSP Commands) is more specific regarding the 41h / 42h registers (Set digitized sound output sampling rate): Valid sampling rates range from 5000 to 45000 Hz inclusive. There is no comment regarding error handling if the register is filled with an out-of-range value. (See also section 3-28 "8-bit or 16-bit Auto-initialize Transfer"). Assume limits are enforced in hardware. This fixes triggering an assertion in audio_calloc(): #1 abort #2 audio_bug audio/audio.c:119:9 #3 audio_calloc audio/audio.c:154:9 #4 audio_pcm_sw_alloc_resources_out audio/audio_template.h:116:15 #5 audio_pcm_sw_init_out audio/audio_template.h:175:11 #6 audio_pcm_create_voice_pair_out audio/audio_template.h:410:9 #7 AUD_open_out audio/audio_template.h:503:14 #8 continue_dma8 hw/audio/sb16.c:216:20 #9 dma_cmd8 hw/audio/sb16.c:276:5 #10 command hw/audio/sb16.c:0 #11 dsp_write hw/audio/sb16.c:949:13 #12 portio_write softmmu/ioport.c:205:13 #13 memory_region_write_accessor softmmu/memory.c:491:5 #14 access_with_adjusted_size softmmu/memory.c:552:18 #15 memory_region_dispatch_write softmmu/memory.c:0:13 #16 flatview_write_continue softmmu/physmem.c:2759:23 #17 flatview_write softmmu/physmem.c:2799:14 #18 address_space_write softmmu/physmem.c:2891:18 #19 cpu_outw softmmu/ioport.c:70:5 [*] http://www.baudline.com/solutions/full_duplex/sb16_pci/index.html OSS-Fuzz Report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29174 Fixes: 85571bc7415 ("audio merge (malc)") Buglink: https://bugs.launchpad.net/bugs/1910603 Tested-by: Qiang Liu Reviewed-by: Qiang Liu Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20210616104349.2398060-1-f4bug@amsat.org> Signed-off-by: Gerd Hoffmann --- MAINTAINERS | 1 + hw/audio/sb16.c | 14 ++++++++++ tests/qtest/fuzz-sb16-test.c | 52 ++++++++++++++++++++++++++++++++++++ tests/qtest/meson.build | 1 + 4 files changed, 68 insertions(+) create mode 100644 tests/qtest/fuzz-sb16-test.c diff --git a/MAINTAINERS b/MAINTAINERS index 636bf2f53655..4842cc26e5ce 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2221,6 +2221,7 @@ F: qapi/audio.json F: tests/qtest/ac97-test.c F: tests/qtest/es1370-test.c F: tests/qtest/intel-hda-test.c +F: tests/qtest/fuzz-sb16-test.c Block layer core M: Kevin Wolf diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c index 8b2070041025..5cf121fe3639 100644 --- a/hw/audio/sb16.c +++ b/hw/audio/sb16.c @@ -115,6 +115,9 @@ struct SB16State { PortioList portio_list; }; +#define SAMPLE_RATE_MIN 5000 +#define SAMPLE_RATE_MAX 45000 + static void SB_audio_callback (void *opaque, int free); static int magic_of_irq (int irq) @@ -241,6 +244,17 @@ static void dma_cmd8 (SB16State *s, int mask, int dma_len) int tmp = (256 - s->time_const); s->freq = (1000000 + (tmp / 2)) / tmp; } + if (s->freq < SAMPLE_RATE_MIN) { + qemu_log_mask(LOG_GUEST_ERROR, + "sampling range too low: %d, increasing to %u\n", + s->freq, SAMPLE_RATE_MIN); + s->freq = SAMPLE_RATE_MIN; + } else if (s->freq > SAMPLE_RATE_MAX) { + qemu_log_mask(LOG_GUEST_ERROR, + "sampling range too high: %d, decreasing to %u\n", + s->freq, SAMPLE_RATE_MAX); + s->freq = SAMPLE_RATE_MAX; + } if (dma_len != -1) { s->block_size = dma_len << s->fmt_stereo; diff --git a/tests/qtest/fuzz-sb16-test.c b/tests/qtest/fuzz-sb16-test.c new file mode 100644 index 000000000000..51030cd7dc40 --- /dev/null +++ b/tests/qtest/fuzz-sb16-test.c @@ -0,0 +1,52 @@ +/* + * QTest fuzzer-generated testcase for sb16 audio device + * + * Copyright (c) 2021 Philippe Mathieu-Daudé + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "libqos/libqtest.h" + +/* + * This used to trigger the assert in audio_calloc + * https://bugs.launchpad.net/qemu/+bug/1910603 + */ +static void test_fuzz_sb16_0x1c(void) +{ + QTestState *s = qtest_init("-M q35 -display none " + "-device sb16,audiodev=snd0 " + "-audiodev none,id=snd0"); + qtest_outw(s, 0x22c, 0x41); + qtest_outb(s, 0x22c, 0x00); + qtest_outw(s, 0x22c, 0x1004); + qtest_outw(s, 0x22c, 0x001c); + qtest_quit(s); +} + +static void test_fuzz_sb16_0x91(void) +{ + QTestState *s = qtest_init("-M pc -display none " + "-device sb16,audiodev=none " + "-audiodev id=none,driver=none"); + qtest_outw(s, 0x22c, 0xf141); + qtest_outb(s, 0x22c, 0x00); + qtest_outb(s, 0x22c, 0x24); + qtest_outb(s, 0x22c, 0x91); + qtest_quit(s); +} + +int main(int argc, char **argv) +{ + const char *arch = qtest_get_arch(); + + g_test_init(&argc, &argv, NULL); + + if (strcmp(arch, "i386") == 0) { + qtest_add_func("fuzz/test_fuzz_sb16/1c", test_fuzz_sb16_0x1c); + qtest_add_func("fuzz/test_fuzz_sb16/91", test_fuzz_sb16_0x91); + } + + return g_test_run(); +} diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index c3a223a83d6a..b03e85417001 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -20,6 +20,7 @@ slow_qtests = { qtests_generic = \ (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? ['fuzz-megasas-test'] : []) + \ (config_all_devices.has_key('CONFIG_VIRTIO_SCSI') ? ['fuzz-virtio-scsi-test'] : []) + \ + (config_all_devices.has_key('CONFIG_SB16') ? ['fuzz-sb16-test'] : []) + \ [ 'cdrom-test', 'device-introspect-test', From 0c29b786e6b5276d43be2be255a8323c628ec790 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 16 Jun 2021 23:14:11 +0900 Subject: [PATCH 6/7] audio: Fix format specifications of debug logs Signed-off-by: Akihiko Odaki Message-id: 20210616141411.53892-1-akihiko.odaki@gmail.com Message-Id: <20210616141411.53892-1-akihiko.odaki@gmail.com> Signed-off-by: Gerd Hoffmann --- audio/audio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/audio/audio.c b/audio/audio.c index 052ca6cb789b..59453ef85673 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -705,7 +705,7 @@ static size_t audio_pcm_sw_write(SWVoiceOut *sw, void *buf, size_t size) if (live == hwsamples) { #ifdef DEBUG_OUT - dolog ("%s is full %d\n", sw->name, live); + dolog ("%s is full %zu\n", sw->name, live); #endif return 0; } @@ -995,7 +995,7 @@ static size_t audio_get_avail (SWVoiceIn *sw) } ldebug ( - "%s: get_avail live %d ret %" PRId64 "\n", + "%s: get_avail live %zu ret %" PRId64 "\n", SW_NAME (sw), live, (((int64_t) live << 32) / sw->ratio) * sw->info.bytes_per_frame ); @@ -1022,7 +1022,7 @@ static size_t audio_get_free(SWVoiceOut *sw) dead = sw->hw->mix_buf->size - live; #ifdef DEBUG_OUT - dolog ("%s: get_free live %d dead %d ret %" PRId64 "\n", + dolog ("%s: get_free live %zu dead %zu ret %" PRId64 "\n", SW_NAME (sw), live, dead, (((int64_t) dead << 32) / sw->ratio) * sw->info.bytes_per_frame); From 986bdbc6a29c4d7ef125299c5013783e30dc2cae Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 16 Jun 2021 23:17:21 +0900 Subject: [PATCH 7/7] coreaudio: Fix output stream format settings Before commit 7d6948cd98cf5ad8a3458a4ce7fdbcb79bcd1212, it was coded to retrieve the initial output stream format settings, modify the frame rate, and set again. However, I removed a frame rate modification code by mistake in the commit. It also assumes the initial output stream format is consistent with what QEMU expects, but that expectation is not in the code, which makes it harder to understand and will lead to breakage if the initial settings change. This change explicitly sets all of the output stream settings to solve these problems. Signed-off-by: Akihiko Odaki Message-Id: <20210616141721.54091-1-akihiko.odaki@gmail.com> Signed-off-by: Gerd Hoffmann --- audio/coreaudio.c | 48 +++++++++++++---------------------------------- 1 file changed, 13 insertions(+), 35 deletions(-) diff --git a/audio/coreaudio.c b/audio/coreaudio.c index 578ec9b8b2e6..f570e1ee60ef 100644 --- a/audio/coreaudio.c +++ b/audio/coreaudio.c @@ -39,7 +39,6 @@ typedef struct coreaudioVoiceOut { int frameSizeSetting; uint32_t bufferCount; UInt32 audioDevicePropertyBufferFrameSize; - AudioStreamBasicDescription outputStreamBasicDescription; AudioDeviceIOProcID ioprocid; bool enabled; } coreaudioVoiceOut; @@ -114,24 +113,6 @@ static OSStatus coreaudio_set_framesize(AudioDeviceID id, UInt32 *framesize) framesize); } -static OSStatus coreaudio_get_streamformat(AudioDeviceID id, - AudioStreamBasicDescription *d) -{ - UInt32 size = sizeof(*d); - AudioObjectPropertyAddress addr = { - kAudioDevicePropertyStreamFormat, - kAudioDevicePropertyScopeOutput, - kAudioObjectPropertyElementMaster - }; - - return AudioObjectGetPropertyData(id, - &addr, - 0, - NULL, - &size, - d); -} - static OSStatus coreaudio_set_streamformat(AudioDeviceID id, AudioStreamBasicDescription *d) { @@ -373,6 +354,17 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) OSStatus status; AudioValueRange frameRange; + AudioStreamBasicDescription streamBasicDescription = { + .mBitsPerChannel = core->hw.info.bits, + .mBytesPerFrame = core->hw.info.bytes_per_frame, + .mBytesPerPacket = core->hw.info.bytes_per_frame, + .mChannelsPerFrame = core->hw.info.nchannels, + .mFormatFlags = kLinearPCMFormatFlagIsFloat, + .mFormatID = kAudioFormatLinearPCM, + .mFramesPerPacket = 1, + .mSampleRate = core->hw.info.freq + }; + status = coreaudio_get_voice(&core->outputDeviceID); if (status != kAudioHardwareNoError) { coreaudio_playback_logerr (status, @@ -432,29 +424,16 @@ static OSStatus init_out_device(coreaudioVoiceOut *core) } core->hw.samples = core->bufferCount * core->audioDevicePropertyBufferFrameSize; - /* get StreamFormat */ - status = coreaudio_get_streamformat(core->outputDeviceID, - &core->outputStreamBasicDescription); - if (status == kAudioHardwareBadObjectError) { - return 0; - } - if (status != kAudioHardwareNoError) { - coreaudio_playback_logerr (status, - "Could not get Device Stream properties\n"); - core->outputDeviceID = kAudioDeviceUnknown; - return status; - } - /* set Samplerate */ status = coreaudio_set_streamformat(core->outputDeviceID, - &core->outputStreamBasicDescription); + &streamBasicDescription); if (status == kAudioHardwareBadObjectError) { return 0; } if (status != kAudioHardwareNoError) { coreaudio_playback_logerr (status, "Could not set samplerate %lf\n", - core->outputStreamBasicDescription.mSampleRate); + streamBasicDescription.mSampleRate); core->outputDeviceID = kAudioDeviceUnknown; return status; } @@ -598,7 +577,6 @@ static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as, qapi_AudiodevCoreaudioPerDirectionOptions_base(cpdo), as, 11610); core->bufferCount = cpdo->has_buffer_count ? cpdo->buffer_count : 4; - core->outputStreamBasicDescription.mSampleRate = (Float64) as->freq; status = AudioObjectAddPropertyListener(kAudioObjectSystemObject, &voice_addr, handle_voice_change,