[mythtv] Ticket #2420: mythreplex segfault

[D. Hugh Redelmeier]

[Warning: I'm brand new and naive here.]

| #2420: mythreplex segfault

| Comment (by paulh):
| 
|  (In [11294]) Possible fix for #2420. find_audio_sync() was always
|  expecting the buf array
|  passed as a parameter to be 7 bytes but some of the calling functions
|  where
|  only creating 4 and 6 byte arrays.

I'm looking at the code.  It looks like it could use some
documentation so that intent could be understood.

My guess is that the function find_audio_sync has a lot of scar
tissue.  The little finite state machine ("found" is the state) looks
overly intricate; perhaps debugging it introduced the redundant
initialization before the initial switch.  As best I can make out, the
memset is not necessary, and it is the only thing that forces buffers
to be 7 long.

|  Also spotted another possible bug where the new fix sync stuff could try
|  to
|  write to the output files before they were opened.

Should that not be a separate changeset?  It seems unrelated.

|  Not sure if it fixes the seg fault or the other reported problems with
|  stack
|  smashing because I couldn't reproduce it even with the sample file
|  provided.

I might be able to do so.  The build system is a bit of a pain.

================

Here's my attempt to simplify find_audio_sync.  I don't know what it
is supposed to do, so I'm kind of limited in what I can accomplish.
The code is not tested, not even compiled.

In theory, it should work without crashing in the cases that the
original crashed.  In particular, it does not need to have the callers
allocate a buffer of length 7.  They need to allocate 6 or 4,
depending on whether the stream is AC3 or MPEG_AUDIO.  I think they
did that before the latest change.

int find_audio_sync(ringbuffer *rbuf, uint8_t *buf, int off, int type, int le)
{
	int c;
	int l;
	uint8_t b1,b2,m2;
	int r;

	switch(type){
	case AC3:
		b1 = 0x0B;
		b2 = 0x77;
		m2 = 0xFF;
		l = 6;
		break;

	case MPEG_AUDIO:
		b1 = 0xFF;
		b2 = 0xF8;
		m2 = 0xF8;
		l = 4;
		break;

	default:
		return -1;
	}

	c = off;
	while ( c-off < le){
		uint8_t b;

		if ((r = mring_peek(rbuf, &b, 1, c)) <0) return -1;
		c++;
		while ( b == b1) {
			if ((r = mring_peek(rbuf, &b, 1, c)) <0) return -1;
			c++;
			if ( (b&m2) == b2 ) {
				if ((r = mring_peek(rbuf, buf, l, c-2)) < -1) 
					return -2;
				return c-2-off;	
			}
		}
	}
	return -1;
}

I don't actually know the development model for MythTV.

Is their someone who "owns" (in some sense) the replex code?

Is it of intererst to improve the abstract quality of the code, or are
only bug fixes interesting?  (Matters of taste can start Holy Wars.)

Does a newbie poking around and making naive suggestions just get
annoying?

Are there unit tests to catch regressions?

Should I be asking this somewhere else (say: IRC)?

Sorry if I didn't listen enough before chattering.