[Tremor] Some issues with Nicholas' comment truncation patch

[Andy Grundman]

OK, I need to make some more test files and see if any of them can be  
read properly.  I may use your patch anyway because it at least avoids  
allocating tons of memory and crashing.  Even if the file doesn't play  
it will fail more gracefully than it currently does.

On Jan 22, 2009, at 10:18 AM, Nicholas Vinen wrote:

>
> Unfortunately while in many cases the patch works, I've found that it
> has problems with certain files. It's a bit of a hack and I don't yet
> fully understand why it doesn't work.
>
> A much better solution would be to examine the header of any packet
> which exceeds a certain size and is not yet fully read in yet, and
> determine the type. If it's the comment field, it should be possible  
> to
> process what's already been read in and sync up to the next packet
> without reading the rest in at all, rather than what I did which is to
> prevent the current packet in memory growing once it reaches a  
> certain size.
>
> So in summary I think somebody needs to come up with a better  
> solution.
> I will do it at some point but I don't know when that will be, it  
> could
> be a while. Someone who understands the Ogg packet code better would
> likely do a good job. I don't yet understand it fully.
>
> You may be able to get my code to work but as it is unfortunately I
> don't think it's 100%. And yes you may be right, the later patch may
> have broken the large comment skipping.
>
>
> Nicholas
>
>
>
> Andy Grundman wrote:
>> Hi Nicholas,
>>
>> I'm working on implementing your comment truncation patch into our
>> Tremor code.  I have a few questions about it.
>>
>> Comparing Tremor3.patch [3] and your original patch [1], #3 seems to
>> have removed the extra code in _vorbis_unpack_comment that breaks out
>> of the comment reading loop if the data was truncated.  Using patch  
>> #3
>> this loop always tries to allocate space for the truncated comment  
>> and
>> fails, so I think what you did in #1 is the right solution.  However,
>> using #1 I end up failing on my test file with ENOTVORBIS because the
>> check for the "vorbis" string in vorbis_dsp_headerin fails when
>> looking for the codebooks header.
>>
>> Thanks,
>> -Andy
>>
>> [1] http://x256.org/~hb/Tremor.patch
>> [2] http://x256.org/~hb/Tremor3.patch