Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions pedalboard/io/PythonFileLike.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#pragma once

#include <cerrno>
#include <mutex>
#include <optional>

Expand Down Expand Up @@ -45,6 +46,31 @@ inline void raise() {
}
}; // namespace PythonException

/**
* @brief A helper class that clears the thread's errno when destructed.
*
* This is used to avoid failure when the following sequence of events occurs:
*
* 1. A Python file-like object is passed to Pedalboard, which could call any
* other native code in its methods
* 2. Pedalboard calls a Python file-like object's methods (e.g. read(), seek(),
* tell(), seekable())
* 3. The native code sets errno to a non-zero value, but does not clear it
* 4. Pedalboard's codecs (Ogg Vorbis, etc) check errno and fail to decode
* 5. The user is presented with a cryptic error message
*
* This makes it seem like we're ignoring errno, but we're not; the Python-level
* code should throw an exception if the file-like object has an error, and we
* handle errors at that level correctly.
*
* See also: https://en.wikipedia.org/wiki/Errno.h
*/
class ClearErrnoBeforeReturn {
public:
ClearErrnoBeforeReturn() {}
~ClearErrnoBeforeReturn() { errno = 0; }
};

/**
* A tiny helper class that downgrades a write lock
* to a read lock for the given scope. If we can't
Expand Down
5 changes: 5 additions & 0 deletions pedalboard/io/PythonInputStream.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class PythonInputStream : public juce::InputStream, public PythonFileLike {

juce::int64 getTotalLength() noexcept override {
ScopedDowngradeToReadLockWithGIL lock(objectLock);
ClearErrnoBeforeReturn clearErrnoBeforeReturn;
py::gil_scoped_acquire acquire;

if (PythonException::isPending())
Expand Down Expand Up @@ -99,6 +100,7 @@ class PythonInputStream : public juce::InputStream, public PythonFileLike {
// sign that something is broken!
jassert(buffer != nullptr && bytesToRead >= 0);
ScopedDowngradeToReadLockWithGIL lock(objectLock);
ClearErrnoBeforeReturn clearErrnoBeforeReturn;

py::gil_scoped_acquire acquire;
if (PythonException::isPending())
Expand Down Expand Up @@ -161,6 +163,7 @@ class PythonInputStream : public juce::InputStream, public PythonFileLike {
juce::int64 totalLength = getTotalLength();

ScopedDowngradeToReadLockWithGIL lock(objectLock);
ClearErrnoBeforeReturn clearErrnoBeforeReturn;
py::gil_scoped_acquire acquire;

if (PythonException::isPending())
Expand All @@ -183,6 +186,7 @@ class PythonInputStream : public juce::InputStream, public PythonFileLike {

juce::int64 getPosition() noexcept override {
ScopedDowngradeToReadLockWithGIL lock(objectLock);
ClearErrnoBeforeReturn clearErrnoBeforeReturn;
py::gil_scoped_acquire acquire;

if (PythonException::isPending())
Expand All @@ -201,6 +205,7 @@ class PythonInputStream : public juce::InputStream, public PythonFileLike {

bool setPosition(juce::int64 pos) noexcept override {
ScopedDowngradeToReadLockWithGIL lock(objectLock);
ClearErrnoBeforeReturn clearErrnoBeforeReturn;
py::gil_scoped_acquire acquire;

if (PythonException::isPending())
Expand Down
2 changes: 1 addition & 1 deletion pedalboard/io/ReadableAudioFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class ReadableAudioFile
} else {
ss << " has its stream position set to the end of the stream ("
<< originalStreamPosition;
ss << "bytes).";
ss << " bytes).";
}
ss << " Try seeking this file-like object back to its start before "
"passing it to AudioFile";
Expand Down
65 changes: 65 additions & 0 deletions tests/test_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -1592,3 +1592,68 @@ def test_write_two_by_two_buffer_with_hint():
f.write(np.zeros((2, 2), dtype=np.float32))

assert f.tell() == 2


class ErrnoTriggeringFileLike:
"""A file-like object that triggers errno to be set through real failed operations."""

def __init__(self, wrapped_file):
self._file = wrapped_file
self.name = getattr(wrapped_file, "name", None)
# Create a path that definitely doesn't exist for triggering ENOENT
self._nonexistent_path = str(pathlib.Path("definitely_does_not_exist_12345.txt"))

def _trigger_errno(self):
# Try to open a non-existent file, which sets errno to ENOENT (2)
# We catch the exception but errno remains set
try:
open(self._nonexistent_path, "rb")
except FileNotFoundError:
pass # Expected - errno is now set to ENOENT

def read(self, *args, **kwargs):
result = self._file.read(*args, **kwargs)
self._trigger_errno()
return result

def seek(self, *args, **kwargs):
result = self._file.seek(*args, **kwargs)
self._trigger_errno()
return result

def tell(self):
result = self._file.tell()
self._trigger_errno()
return result

def seekable(self):
result = self._file.seekable()
self._trigger_errno()
return result


@pytest.mark.parametrize("format_name", ["wav", "ogg"])
def test_errno_cleared_after_python_file_operations(format_name: str):
"""
Regression test to ensure that errno values set by Python file-like objects
don't leak through and cause codec failures. Before the fix (ClearErrnoBeforeReturn),
errno values set during Python file operations would cause mysterious failures in
codecs like Ogg Vorbis that check errno.

This test creates a file-like wrapper that intentionally sets errno to a non-zero
value after each operation, simulating what might happen with certain third-party
libraries or file systems. The fix ensures these errno values are cleared before
returning to codec code.
"""

buf = io.BytesIO()
buf.name = f"test.{format_name}"
with pedalboard.io.AudioFile(buf, "w", 44100, 2, format=format_name) as af:
af.write(np.random.rand(2, 44100))

# Add some random garbage past the end of the file to trigger the bug:
buf.write(b"\x00\x00")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only reliable way I could find to repro the bug in test, apart from including a proprietary audio file as part of the tests. Trailing null bytes should not prevent the file from being parsed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's supposed to trigger the bug but now this PR fixed it, should we actually test this with a simple

    with pedalboard.io.AudioFile(buf) as af:  # <- the buffer with garbage in it.. which should now succeed parsing  without any change
        assert af.samplerate == 44100  

and we can keep the other assert with a different but perfectly fine buffer

    with pedalboard.io.AudioFile(ErrnoTriggeringFileLike(ok_buffer_made_not_ok_with_wrapper_class)) as af:
        assert af.samplerate == 44100

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I fully follow - the code here does fail on Linux if I comment out the errno = 0 fix in the code under test.

Copy link
Copy Markdown
Collaborator

@hyperc54 hyperc54 Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code here does fail on Linux if I comment out the errno = 0 fix in the code under test.

This makes sense 👍

What I was challenging is this part:

    # Add some random garbage past the end of the file to trigger the bug:
    buf.write(b"\x00\x00")

I would argue you don't need to add random garbage because the test buffer is wrapped in ErrnoTriggeringFileLike which would trigger the bug in any case with any buffer, clean or not. (If I understand the class properly)

I was then suggesting to actually test without the ErrnoTriggeringFileLike and an unhealthy buffer :)

not important though!

buf.seek(0)

with pedalboard.io.AudioFile(ErrnoTriggeringFileLike(buf)) as af:
assert af.samplerate == 44100
Loading