google / google/magika

python: identify_stream() raises UnboundLocalError instead of the real OSError on unseekable streams

Open
#1,473 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
18.6k
Forks
1.2k
Avg merge
3d 12h
Merged PRs (30d)
20

Description

### Summary

`Magika.identify_stream()` raises `UnboundLocalError` instead of propagating the real I/O error when the input stream passes every type guard but its `tell()` fails (i.e. a readable-but-unseekable `io.BufferedIOBase`, such as `sys.stdin.buffer` on a pipe or `socket.makefile("rb")`). The real `OSError: [Errno 29] Illegal seek` is discarded as the *context* of a bogus internal error.

### Environment

- `main` @ `e6a4c8e` ("Expose the Rust library as a C library (#1449)")
- `python/` package version `1.0.3`, Python 3.13, macOS arm64

### Reproduction

```python
import io, os
from magika import Magika

m = Magika()

rp, wp = os.pipe()
os.write(wp, b"hello world this is a test\n" * 10)
os.close(wp)
buf = io.BufferedReader(io.FileIO(rp, closefd=True))

print(isinstance(buf, io.BufferedIOBase), buf.readable(), buf.seekable())
# True True False -> passes every guard in identify_stream()

m.identify_stream(buf)
```

Equivalent one-liner:

```bash
echo hi | python3 -c "import sys; from magika import Magika; Magika().identify_stream(sys.stdin.buffer)"
```

### Actual result

```
True True False
Traceback (most recent call last):
File ".../magika/magika.py", line 204, in identify_stream
current_position = stream.tell()
OSError: [Errno 29] Illegal seek

During handling of the above exception, another exception occurred:
...
UnboundLocalError: cannot access local variable 'current_position' where it is not associated with a value
```

### Expected result

The original `OSError` (unseekable stream) propagates unchanged. Magika's own error should never shadow the underlying I/O failure.

### Root cause

`python/src/magika/magika.py:203-209`:

```python
try:
current_position = stream.tell()
result = self._get_result_from_seekable(Seekable(stream))
finally:
# seek to the previous position even in case of exceptions
stream.seek(current_position)
return result
```

`current_position` is bound *inside* the `try` block but referenced unconditionally in `finally`. When the first statement (`stream.tell()`) raises, the name was never bound, so `finally` raises `UnboundLocalError` and replaces the real exception.

### Impact

Callers cannot distinguish "this stream is not seekable" from a Magika internal bug: the traceback names a Magika local variable and the actionable `OSError` is demoted to `__context__`. This is the exact path taken by anyone piping data into a Python tool that forwards `sys.stdin.buffer` to `identify_stream()`.

### Proposed fix

Hoist the `tell()` out of the `try`, so that `finally` only runs once the position is known:

```python
current_position = stream.tell()
try:
result = self._get_result_from_seekable(Seekable(stream))
finally:
stream.seek(current_position)
return result
```

The position-restoring guarantee is preserved, because the only statement remaining inside `try` is the one that needs the restore.

I have a fix plus a regression test in `python/tests/test_magika_python_module.py` (next to `test_magika_module_identify_stream_does_not_alter_position`) and will open a PR shortly.

Contributor guide

Open the contributing guide

Research direction

Start in python/src/magika/magika.py around identify_stream() lines 203-209, then read the nearby position-preservation test in python/tests/test_magika_python_module.py. Run the existing stream tests and add coverage for tell() failing on an unseekable stream; done means the original OSError propagates without an UnboundLocalError.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.