sndiod(1) regression, Segmentation Fault on recorging

[Dimitri Sokolyuk]

>Synopsis: sndiod(1) regression, Segmentation Fault on recorging
>Category: system
>Environment:
System : OpenBSD 5.3
Details : OpenBSD 5.3 (GENERIC.MP) #62: Tue Mar 12 18:21:20 MDT
2013
der...@amd64.openbsd.org:
/usr/src/sys/arch/amd64/compile/GENERIC.MP

Architecture: OpenBSD.amd64
Machine : amd64
>Description:
sndiod(1) dies with Segmentation Fault on data recording.
The bug was introduced in 5.3 release. The crash occurs on line 628
of dsp.c
>How-To-Repeat:
Run minimal example below.
>Fix:
unknown

Minimal example:

#include <assert.h>
#include <sndio.h>
#include <stdlib.h>

int
main(int argc, char **argv)
{
struct sio_hdl *sio;
struct sio_par par;

int16_t *buffer;
size_t bufsz;

sio = sio_open(NULL, SIO_REC, 0); /* record */
assert(sio);

sio_initpar(&par);
sio_getpar(sio, &par);

bufsz = par.round * sizeof(int16_t);
buffer = malloc(bufsz);
assert(buffer);

sio_start(sio);

sio_read(sio, buffer, bufsz); /* sndiod(1) dies here */
assert(sio_eof(sio) == 0);

sio_stop(sio);
sio_close(sio);

free(buffer);

return 0;
}

[Alexandre Ratchov]

On Sun, May 05, 2013 at 12:15:43AM +0200, Dimitri Sokolyuk wrote:
> >Synopsis: sndiod(1) regression, Segmentation Fault on recorging
> >Category: system
> >Environment:
> System : OpenBSD 5.3
> Details : OpenBSD 5.3 (GENERIC.MP) #62: Tue Mar 12 18:21:20 MDT
> 2013
> der...@amd64.openbsd.org:
> /usr/src/sys/arch/amd64/compile/GENERIC.MP
>
> Architecture: OpenBSD.amd64
> Machine : amd64
> >Description:
> sndiod(1) dies with Segmentation Fault on data recording.
> The bug was introduced in 5.3 release. The crash occurs on line 628
> of dsp.c
> >How-To-Repeat:
> Run minimal example below.

Thanks for the report and the test program. It should be fixed in
-current (in dev.c rev 1.5). The diff below applies to 5.3 as well.

FWIW, there weren't audio related api changes, so it's safe to
build libsndio and sndiod from -current sources on 5.3.

-- Alexandre

Index: dev.c
===================================================================
RCS file: /var/anoncvs/sndio/sndio/sndiod/dev.c,v
retrieving revision 1.41
diff -u -p -r1.41 dev.c
--- dev.c 26 Feb 2013 11:18:41 -0000 1.41
+++ dev.c 5 May 2013 09:15:46 -0000
@@ -1574,6 +1574,7 @@ found:
s->dup = 0;
s->appbufsz = d->bufsz;
s->round = d->round;
+ s->rate = d->rate;
dev_midi_slotdesc(d, s);
dev_midi_vol(d, s);
return s;

[Dimitri Sokolyuk]

Thanks Alexandre, the patch fixed the original issue.

However I just noted another regression: in a monitoring mode only a left
channel is copied back now.

Since it is not as easy to demonstrate as the last issue, here is a link to
my little-ugly-home-brewed sound visualizer:
http://www.dim13.org/cgi-bin/cvsweb/src/spectrogram/ (requires
sndiod_flags="-m play,mon,midi" and some sound source like mplayer) which
demonstrates the issue.

Thanks & wbr Dimitri Sokolyuk

[Alexandre Ratchov]

nice program! it doesn't set initial audio parameters which exposes
few uninitialized variables in sndiod. Does this diff fix works for
you?

-- Alexandre

Index: dev.c
===================================================================
RCS file: /var/anoncvs/sndio/sndio/sndiod/dev.c,v

retrieving revision 1.42
diff -u -p -r1.42 dev.c
--- dev.c 5 May 2013 16:19:51 -0000 1.42
+++ dev.c 5 May 2013 17:16:32 -0000
@@ -1560,21 +1560,22 @@ found:
return 0;
}
s->mode = mode;
- s->par = d->par;
+ aparams_init(&s->par);
if (s->mode & MODE_PLAY) {
- s->mix.slot_cmin = 0;
- s->mix.slot_cmax = d->pchan - 1;
+ s->mix.slot_cmin = s->mix.dev_cmin = 0;
+ s->mix.slot_cmax = s->mix.dev_cmax = d->pchan - 1;
}
if (s->mode & MODE_RECMASK) {
- s->sub.slot_cmin = 0;
- s->sub.slot_cmax = ((s->mode & MODE_MON) ?
- d->pchan : d->rchan) - 1;
+ s->sub.slot_cmin = s->sub.dev_cmin = 0;
+ s->sub.slot_cmax = s->sub.dev_cmax =
+ ((s->mode & MODE_MON) ? d->pchan : d->rchan) - 1;
}
s->xrun = XRUN_IGNORE;

s->dup = 0;
s->appbufsz = d->bufsz;
s->round = d->round;

s->rate = d->rate;

+ s->mix.maxweight = ADATA_UNIT;

dev_midi_slotdesc(d, s);
dev_midi_vol(d, s);
return s;

Index: sock.c
===================================================================
RCS file: /var/anoncvs/sndio/sndio/sndiod/sock.c,v
retrieving revision 1.30
diff -u -p -r1.30 sock.c
--- sock.c 6 Dec 2012 08:12:00 -0000 1.30
+++ sock.c 5 May 2013 17:16:32 -0000
@@ -670,13 +670,13 @@ sock_setpar(struct sock *f)
if (log_level >= 3) {
sock_log(f);
log_puts(": recording channels ");
- log_putu(s->sub.slot_cmin);
- log_puts(":");
- log_putu(s->sub.slot_cmax);
- log_puts(" -> ");
log_putu(s->sub.dev_cmin);
log_puts(":");
log_putu(s->sub.dev_cmax);
+ log_puts(" -> ");
+ log_putu(s->sub.slot_cmin);
+ log_puts(":");
+ log_putu(s->sub.slot_cmax);
log_puts("\n");
}
#endif
@@ -907,14 +907,13 @@ sock_hello(struct sock *f)
if (s == NULL)
return 0;
f->midi = NULL;
- aparams_init(&s->par);
if (s->mode & MODE_PLAY) {
- s->mix.slot_cmin = f->opt->pmin;
- s->mix.slot_cmax = f->opt->pmax;
+ s->mix.slot_cmin = s->mix.dev_cmin = f->opt->pmin;
+ s->mix.slot_cmax = s->mix.dev_cmax = f->opt->pmax;
}
if (s->mode & MODE_RECMASK) {
- s->sub.slot_cmin = f->opt->rmin;
- s->sub.slot_cmax = f->opt->rmax;
+ s->sub.slot_cmin = s->sub.dev_cmin = f->opt->rmin;
+ s->sub.slot_cmax = s->sub.dev_cmax = f->opt->rmax;
}
if (f->opt->mmc) {
s->xrun = XRUN_SYNC;

[Dimitri Sokolyuk]

Thank you very much! The problem is gone now.

Regarding uninitialized parameters, could you please point out, which of
them are essential? I'm not sure it is stressed in the man-page. (Certainly
I've overseen it.)

wbr
Dimtri Sokolyuk

[Alexandre Ratchov]

On Sun, May 05, 2013 at 10:16:30PM +0200, Dimitri Sokolyuk wrote:
> Thank you very much! The problem is gone now.
>
> Regarding uninitialized parameters, could you please point out, which of
> them are essential? I'm not sure it is stressed in the man-page. (Certainly
> I've overseen it.)

The program is supposed to ask the device for its prefered
parameters and then check if the device has accepted them:

AFAICS, the spectrogram program assumes samples are stereo 16-bit
native byte order signed integers. For instance it could do
(pseudo-code):

sio_initpar(&par);
par.rchan = 2;
par.bits = 16;
par.le = SIO_NATIVE_LE;
par.sig = 1;
if (!sio_setpar(hdl, &par))
errx(1, "couldn't set params\n");
if (!sio_getpar(hdl, &par))
errx(1, "couldn't get params\n");
if (par.rchan != 2 ||
par.bits != 16 ||
par.le != SIO_NATIVE_LE ||
sio.sig != 1)
errx(1, "unsupported audio params\n");

This ensures that either the device accepts them or the program
exits.

BTW, even if the program can deal with any block size and any
sample rate, setting them to values that makes the animation look
nicer or whatever may be desirable.

Hmm.. may be this should be explained in the man page.

-- Alexandre

[Dimitri Sokolyuk]

Thank you very much again!

An example in the man page would be surely very helpful.

wbr
Dimitri Sokolyuk