[Tremor] alignment issues in ov_read() [was: Re: different tremor output on armel]

[Daniel Kahn Gillmor]

On 09/20/2009 07:11 PM, Daniel Kahn Gillmor wrote:
> This got no response the first time i posted it. can anyone on this list
> confirm successful use of tremor on little-endian ARM (armel)?

Thanks for the helpful responses, everyone.  And big thanks to Martin
Guy, who pointed me in the right direction on fixing this.

there are a series of problems, but they ultimately can trigger an
alignment error (SIGBUS, if you "echo 5 > /proc/cpu/alignment") on line
1858 of vorbisfile.c (in the ov_read function).

the problem?  ov_read is documented as:

   It returns up to the specified number of bytes of decoded audio
   in host-endian, signed 16 bit PCM format.

But buffer is a char*, not a short*.

As a result, it's possible to pass a misaligned buffer to ov_read()
(i.e. one on an odd memory address), and the library will try to do an
unaligned assignment (i.e. it will write a 2-byte short to an odd memory
address).

The right thing to do for the long term seems to be to make ov_read()
accept a short* instead of a char*, but i think that would change the
API of the library, potentially causing a version bump; ugh.

So i'm attaching two patches which "fix" the problem i was having with
ivorbisfile_example.c in two ways:

The first patch simply patches ivorbisfile_example.c to declare the
buffer as an array of shorts (thereby allocating with the correct
alignment), and then casts it back to a char* in the ov_read call.  It
also checks that OV_EINVAL doesn't come back from the call to ov_read
(it shouldn't, according to the logic of ivorbisfile_example.c)

The second patch checks the alignment of the buffer in ov_read and
returns OV_EINVAL if it is not properly aligned.

i'm applying the first patch to the debian version of the package, just
to fix the examples, and i strongly recommend applying it upstream as
well.  i'm not sure what to do about the second patch.

Suggestions?

	--dkg

PS one could argue that gcc shouldn't place the pcmout array on an odd
memory address at all; if you think that's the case, feel free to make
that argument to the gcc folks (and cc me on it).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: align-ivorbisfile_example-pcmout.diff
Type: text/x-diff
Size: 1228 bytes
Desc: not available
Url : http://lists.xiph.org/pipermail/tremor/attachments/20090928/7244f5cf/attachment.diff 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ov_read-checks-buffer-alignment.diff
Type: text/x-diff
Size: 1853 bytes
Desc: not available
Url : http://lists.xiph.org/pipermail/tremor/attachments/20090928/7244f5cf/attachment-0001.diff 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 891 bytes
Desc: OpenPGP digital signature
Url : http://lists.xiph.org/pipermail/tremor/attachments/20090928/7244f5cf/attachment.pgp