Enabling the following function in tests/noop.c:
noop_test_decode ()
{
theora_info ti;
theora_state th;
theora_info_init (&ti);
theora_decode_init (&th, &ti);
theora_clear (&th);
theora_info_clear (&ti);
}
segfaults.
The cause is that theora_decode_init() expects a theora_info structure
which was previously initialized by passing actual bitstream data
through theora_decode_header(), as documented in theora.h:
/**
* Initialize a theora_state handle for decoding.
* \param th The theora_state handle to initialize.
* \param c A theora_info struct filled with the desired decoding parameters.
* This is of course usually obtained from a previous call to
* theora_decode_header().
* \retval 0 Success
*/
int theora_decode_init(theora_state *th, theora_info *c);
Ok, so the test is buggy, it isn't providing the expected data.
Wait a minute. libtheora, and thus applications linking to it, shouldn't
crash if they are passed bad data -- or in this case, no data at all.
Perhaps the sequence of functions in noop_test_decode() is reasonable for
an application that sets up a decoder, loses its data connection then
tries to clean up.
So, I think a baseline test like this noop one is useful in order to
check that setup and teardown of the data structures works reliably. Once
we know that the setup works reliably, we can narrow down problems in the
actual functionality.
Thoughts? Do we fix the test or the library? ;-)
btw. The crash is in theora_decode_init(). Commenting out the call to
theora_info_init() adds a sequence of awesome calls like this to the
output of valgrind:
==28684== Warning: silly arg (-1645612232) to malloc()
cheers,
Conrad.
On 8/5/05, Conrad Parker <conrad@metadecks.org> wrote:> Enabling the following function in tests/noop.c: > So, I think a baseline test like this noop one is useful in order to > check that setup and teardown of the data structures works reliably. Once > we know that the setup works reliably, we can narrow down problems in the > actual functionality. > > Thoughts? Do we fix the test or the library? ;-)Where practical, I think the library shouldn't crash due to bad input. Fix the library. That's often going to be hard, though - there are limits to what we can do in the presence of arbitrary uninitialised data, as in your second example (with theora_info_init() removed) This is one reason why functions like theora_info_init() would be better replaced with something more like theora_info_create(), which would actually allocate the function. In such a case we'd then either get a NULL pointer (easily detected; can return an error code), a good pointer (fine), or a bad pointer (again, nothing we could reasonably do about that). theora_decode_init() shouldn't be crashing solely due to not having a fully initialised theora_info, though - that's a case we can detect without too much hassle. Mike
On 8/5/05, Timothy B. Terriberry <tterribe@xiph.org> wrote:> Michael Smith wrote: > > Where practical, I think the library shouldn't crash due to bad input. > > Fix the library. > > My personal opinion is that if you do stupid things, the library should > blow up (and severely) as soon as possible. So crashing seems perfectly > reasonable to me.Perhaps. I guess that the tendency of programmers to never bother actually checking return values is a point against trying to just return an error value.> > Now, about redesigning the API to allocate structures for you (to make > such blowups happen in an easier-to-check-for place, etc.), well, I > think that's a fantastic idea. In fact theora-exp already does this for > structures that are not solely data containers. It has the other nice > benefit that you can change the internal size and workings without > having to rev the API version and dynamically linked code still works. > But Ralph and others currently want to punt on introducing API changes > until after 1.0. I believe I've already expressed my own opinion that > beta1 is the best opportunity to give libtheora a sane API. >Agreed - the "alpha" designation pretty clearly says "this isn't stable, and things might change", at least to me. I think at the very least we should provide variants that allocate these structures for you, and deprecate the existing functions (but still keep them, unless there's a compelling reason not to) - but keep them up to and including the final 1.0 release. Perhaps a documentation note saying that binary incompatibilities may be introduced if you use the deprecated versions? I guess there's a fair bit of work to be done to make that truly useful, though - the theora_info struct, for instance, has lots of encode-side parameters that a future encoder revision might want to remove or add to, but given the current API this needs to be a public structure (rather than merely an opaque pointer) for access to the decode-side members like width/height/etc. So I'm guessing there'd be a fair bit of internal reorganisation to do to really get to the point where we can revise all these structures without badly breaking binary compatibility. Mike