enthought / enthought/traits

ctraits Extension Analysis Report

Open
#1,881 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
462
Forks
90
PR merge metrics
No merged PRs in 30d

Description

I've run [cext-review-toolkit](https://github.com/devdanzin/cext-review-toolkit) on traits and got a report that includes the issues below:

**1. Use-after-free in `setattr_delegate` — borrowed dict ref used after DECREF**
- **Location**: `ctraits.c:2587-2597`
- **Impact**: `has_traits_getattro` returns new ref -> immediate `Py_DECREF` -> `delegate` is dangling -> subsequent use is use-after-free. The analogous code in `getattr_delegate` and `_has_traits_trait` correctly keeps the reference alive.

**2. Spurious `Py_DECREF(name)` on borrowed parameter in `setattr_trait`**
- **Location**: `ctraits.c:2526`
- **Impact**: `name` is borrowed from `tp_setattro` caller. DECREF on `PyDict_SetItem` error path corrupts refcount, causing use-after-free in the caller.

**3. Unbounded pickle indices in `_trait_setstate` — potential arbitrary code execution**
- **Location**: `ctraits.c:4923-4929`
- **Impact**: Indices from untrusted pickle data index into function pointer arrays without bounds checks. Out-of-bounds read yields garbage function pointer -> arbitrary code execution when called. Compare with `trait_new` which correctly bounds-checks.

**4. `_trait_setstate` backward compat: NULL deref + double-INCREF leak**
- **Location**: `ctraits.c:4939-4950`
- **Impact**: `PyObject_GetAttrString` can return NULL -> `Py_INCREF(NULL)` crashes. When it succeeds, the new reference + unconditional `Py_INCREF` = double reference that leaks.

**5. `trait_clone` leaks old references when called on existing objects**
- **Location**: `ctraits.c:4773-4794`
- **Impact**: Overwrites 6 `PyObject*` fields (handler, py_validate, py_post_setattr, default_value, delegate_name, delegate_prefix) without XDECREF'ing old values. Leaks up to 6 refs per `clone()` call. Compare with `_trait_set_default_value` which uses the correct INCREF-new/XDECREF-old pattern.

**6. `_trait_set_property` leaks old `delegate_name`, `delegate_prefix`, `py_validate`**
- **Location**: `ctraits.c:4758-4763`
- **Impact**: Same class as Finding 5. Overwrites 3 fields without releasing old values.

**7. `_trait_delegate` leaks old `delegate_name` and `delegate_prefix`**
- **Location**: `ctraits.c:4616-4627`
- **Impact**: Same class as Findings 5-6.

**8. `has_traits_new` leaks `obj` on all 3 error paths**
- **Location**: `ctraits.c:694-710`
- **Impact**: `tp_new` succeeds but validation fails -> 3 `return NULL` without `Py_DECREF(obj)`.

**9. `get_trait`: unchecked `PyType_GenericAlloc` + 2 leak paths for `itrait`**
- **Location**: `ctraits.c:950-978`
- **Impact**: NULL deref if alloc fails; `itrait` leaked if `PyList_New` or `PyDict_SetItem` fails.

**10. `has_traits_init` passes NULL `PyDict_GetItem` result to `PyMapping_Size`**
- **Location**: `ctraits.c:729-731`
- **Impact**: `PyDict_GetItem` returns NULL -> `PyMapping_Size(NULL)` -> crash.

**11. Unguarded `PyErr_Clear()` calls in `validate_trait_complex` (4 sites)**
- **Location**: `ctraits.c:4065,4072,4132,4142`
- **Impact**: Cases 5 (enum), 6 (mapped), 12 (castable), 13 (function validator) all swallow `MemoryError`/`KeyboardInterrupt`.

**12. Unguarded `PyErr_Clear()` in `delegate_attr_name_class_name`**
- **Location**: `ctraits.c:4584`
- **Impact**: `MemoryError` from `PyObject_GetAttr` silently swallowed, returns wrong delegate name.

**13. `get_prefix_trait` ignores `PyDict_SetItem` failure + dereferences potential NULL**
- **Location**: `ctraits.c:630,637`
- **Impact**: `PyDict_SetItem` failure ignored (exception clobbered by subsequent calls); `get_trait` NULL return passed to `Py_DECREF`.

**14. `setattr_property0` leaks `args` tuple on call failure**
- **Location**: `ctraits.c:2668-2675`

**15. `_trait_getstate` stores unchecked `PyLong_FromLong` NULLs in tuple**
- **Location**: `ctraits.c:4867-4897`

**16. `PyModule_AddObject` error paths leak type refs + module object**
- **Location**: `ctraits.c:5719-5735`

**17. Unchecked `PyUnicode_FromString` in init (5 sites)**
- **Location**: `ctraits.c:5738-5750`

**18. Unchecked `PyType_GenericNew` in `trait_new`**
- **Location**: `ctraits.c:2968-2970`

**19. `validate_trait_tuple_check` leaks `aitem` when tuple allocation fails**
- **Location**: `ctraits.c:3697-3700`

**20. `setattr_delegate` recursion error leaks `daname`**
- **Location**: `ctraits.c:2646-2648`

The main reproducers are:

## Reproducer 1: Segfault from `setattr_delegate` Use-After-Free via Property Delegate

**Bug**: `setattr_delegate` calls `has_traits_getattro` to get the delegate object (returns new ref with refcount 1), then immediately calls `Py_DECREF` (refcount 0, object freed). The freed `delegate` pointer is then used for `PyHasTraits_Check`, `delegate_attr_name`, and dict lookups — all use-after-free. Triggered when a delegate is a property that creates a fresh object on each access.

**File**: `ctraits.c:2587-2597`

```python
from traits.api import HasTraits, Int, DelegatesTo, Property

class Inner(HasTraits):
value = Int(42)

class Outer(HasTraits):
inner = Property(lambda self: Inner())
value = DelegatesTo("inner")

Outer().value = 99 # Segmentation fault (core dumped)
```

**Output**:
```
Segmentation fault (core dumped)
```

---

## Reproducer 2: Segfault from `clone()` with Handler Reassignment

**Bug**: `trait_clone` overwrites `handler` and 5 other `PyObject*` fields without XDECREF'ing old values. When called repeatedly with different handlers, the old handler's refcount becomes corrupt, leading to a crash.

**File**: `ctraits.c:4773-4794`

```python
from traits.ctraits import cTrait

target = cTrait(0)
for i in range(5000):
source = cTrait(0)
source.handler = lambda: i
target.clone(source)
# Segmentation fault (core dumped)
```

**Output**:
```
Segmentation fault (core dumped)
```

---

## Reproducer 3: Segfault from Unchecked Allocation Under OOM (`get_trait`)

**Bug**: `get_trait` calls `PyType_GenericAlloc` (line 950) and `PyList_New` (line 958) without NULL checks. When allocation fails under memory pressure, the NULL pointer is dereferenced, crashing the interpreter. Triggered using `_testcapi.set_nomemory` to inject OOM at successive allocation points.

**File**: `ctraits.c:950-960`

```python
import _testcapi
from traits.api import HasTraits, Int, on_trait_change

class Obj(HasTraits):
x = Int()

@on_trait_change("x")
def _x_changed(self, new):
pass

for n in range(1, 500):
_testcapi.set_nomemory(n, 0)
try:
obj = Obj()
obj.x = 42
_testcapi.remove_mem_hooks()
except MemoryError:
_testcapi.remove_mem_hooks()
except:
_testcapi.remove_mem_hooks()
# Segmentation fault (core dumped)
```

**Output**:
```
Segmentation fault (core dumped)
```

Please consult https://gist.github.com/devdanzin/c997faaee7556ffc5cb991deca553966 for the full report which includes more issues and reproducers.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.