Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

fabzzap
Copy link
Contributor

@fabzzap fabzzap commented Oct 3, 2015

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
Copy link

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
Copy link

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

@fabzzap
Copy link
Contributor Author

fabzzap commented Oct 8, 2015

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

@fabzzap
Copy link
Contributor Author

fabzzap commented Oct 10, 2015

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
Copy link

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
Copy link
Contributor Author

fabzzap commented Oct 11, 2015

The signature for createTemporaryFile has changed in 34c2610 . 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
Copy link

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
Copy link

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
Copy link

Thanks! Looks like @pSub found it (NixOS/nixpkgs#10829).

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

@mpruett
Copy link
Owner

mpruett commented Jul 5, 2016

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

@mpruett mpruett closed this Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants