mpruett / audiofile

Audio File Library
https://audiofile.68k.org/
GNU Lesser General Public License v2.1
155 stars 41 forks source link

Do not corrupt files when changing both number of channels and sample… #25

Closed fabzzap closed 8 years ago

fabzzap commented 9 years ago

Example: open a 16-bit stereo file and convert it to mono 32-bit by using afSetVirtualChannels(fh, AF_DEFAULT_TRACK, 1); afSetVirtualSampleFormat(fh, AF_DEFAULT_TRACK, AF_SAMPFMT_TWOSCOMP, 32); The resulting samples read with afReadFrames will be corrupted.

mschwendt commented 8 years ago

After applying the patch, your modified test-case still throws an error (before "expected 1, got -101"), now:

$ ./sixteen-stereo-to-eight-mono error expected 28, got 29

What am I missing?

setharnold commented 8 years ago

This has been assigned CVE-2015-7747 by MITRE: http://www.openwall.com/lists/oss-security/2015/10/08/1

fabzzap commented 8 years ago

mschwendt: maybe different compilers round to integers in different ways? I get the same error on Linux, but on Windows the test passes

fabzzap commented 8 years ago

The version posted on Launchpad has 392 replaced with 792 in the initialisation of frames16 (as well as some minor other changes), and you get the error "expected 28, got 29". The version in this pull request does not give the error

mschwendt commented 8 years ago

Progress! The version in this pull request doesn't compile:

sixteen-stereo-to-eight-mono.c: In function ‘main’:
sixteen-stereo-to-eight-mono.c:61:47: warning: passing argument 2 of ‘createTemporaryFile’ from incompatible pointer type [-Wincompatible-pointer-types]
  if (!createTemporaryFile("sixteen-to-eight", &testFileName))
                                               ^
In file included from sixteen-stereo-to-eight-mono.c:41:0:
TestUtilities.h:56:6: note: expected ‘char *’ but argument is of type ‘char **’
 bool createTemporaryFile(const char *prefix, char *path);
      ^
/home/misc/tmp/cc0aT5bG.o: In function `main':
sixteen-stereo-to-eight-mono.c:(.text+0x9e): undefined reference to `createTemporaryFile'

However, fetching the code from launchpad and replacing 792 with 392, it compiles and passes the test without errors.

fabzzap commented 8 years ago

The signature for createTemporaryFile has changed in https://github.com/mpruett/audiofile/commit/34c261034f1193a783196618f0052112e00fbcfe . The file in Launchpad is made so it compiles with version 0.3.6 (the one distributed with Ubuntu currently) and the file in this pull request uses the signature for master HEAD.

falsifian commented 8 years ago

Pardon my ignorance, but where can I find the patch on launchpad?

I am trying to patch audiofile 0.3.6 in the nixpkgs distribution, and I found bug 1502721 on launchpad, but I couldn't find a patch attached to it.

setharnold commented 8 years ago

On Oct 31, 2015 18:36, "James Cook" notifications@github.com wrote:

Pardon my ignorance, but where can I find the patch on launchpad?

I am trying to patch audiofile 0.3.6 in the nixpkgs distribution, and I found bug 1502721 on launchpad, but I couldn't find a patch attached to it.

Hi James, the patch shipped by Ubuntu can be found in the Debian patches tarball linked from the package's pages, e.g. https://launchpad.net/ubuntu/+source/audiofile/0.3.6-2ubuntu0.15.04.1

If you have an Ubuntu system (or any Debian-derivative with correct deb-src lines in the apt configuration) you can just use apt-get source audiofile

Thanks

falsifian commented 8 years ago

Thanks! Looks like @pSub found it (https://github.com/NixOS/nixpkgs/pull/10829).

nixpkgs isn't apt-based, but we are grateful for the patches that come from Debian and Ubuntu.

mpruett commented 8 years ago

Fabrizio, thanks very much for identifying and solving this problem. This change is applied in 49103e386808042f830c18365976ad40875923ea.