From b036ef6eb4ec6074061bb7aacbfb1dfc92504f32 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Tue, 9 Sep 2025 08:34:17 +0200 Subject: [PATCH 1/7] Use PyModuleAddObjectRef instead of Py_INCREF --- src/sequali/_qcmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sequali/_qcmodule.c b/src/sequali/_qcmodule.c index 69fa888..00a098b 100644 --- a/src/sequali/_qcmodule.c +++ b/src/sequali/_qcmodule.c @@ -6044,11 +6044,10 @@ python_module_add_type_spec(PyObject *module, PyType_Spec *spec) return NULL; } - if (PyModule_AddObject(module, class_name, (PyObject *)type) != 0) { + if (PyModule_AddObjectRef(module, class_name, (PyObject *)type) != 0) { Py_DECREF(type); return NULL; } - Py_INCREF((PyObject *)type); return type; } From 2981199e724ea9fa006b0d1bec2b2341df4b297d Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Tue, 9 Sep 2025 08:51:47 +0200 Subject: [PATCH 2/7] Fix memory leaks in module initialization --- src/sequali/_qcmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sequali/_qcmodule.c b/src/sequali/_qcmodule.c index 00a098b..cbba2b4 100644 --- a/src/sequali/_qcmodule.c +++ b/src/sequali/_qcmodule.c @@ -6014,6 +6014,7 @@ ImportClassFromModule(const char *module_name, const char *class_name) return NULL; } PyObject *type_object = PyObject_GetAttrString(module, class_name); + Py_DECREF(module); if (type_object == NULL) { return NULL; } @@ -6094,7 +6095,6 @@ _qc_exec(PyObject *module) if (tp == NULL) { return -1; } - Py_INCREF((PyObject *)tp); address[0] = tp; } @@ -6218,7 +6218,7 @@ _qc_clear(PyObject *mod) size_t number_of_types = sizeof(struct QCModuleState) / sizeof(PyTypeObject *); for (size_t i = 0; i < number_of_types; i++) { - Py_DECREF(mod_state_types[i]); + Py_XDECREF(mod_state_types[i]); mod_state_types[i] = NULL; } return 0; From a99c3f8fe52694ea36d5d8f89e271c7a05af358b Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Tue, 9 Sep 2025 08:58:49 +0200 Subject: [PATCH 3/7] Return empty array when buffersize=0 otherwise uninitialized memory is dereferenced (for some unknown reason). --- src/sequali/_qcmodule.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sequali/_qcmodule.c b/src/sequali/_qcmodule.c index cbba2b4..b22516d 100644 --- a/src/sequali/_qcmodule.c +++ b/src/sequali/_qcmodule.c @@ -126,6 +126,10 @@ PythonArray_FromBuffer(char typecode, void *buffer, size_t buffersize, if (array == NULL) { return NULL; } + if (buffersize == 0) { + /* Return empty array */ + return array; + } /* We cannot paste into the array directly, so use a temporary memoryview */ PyObject *tmp = PyMemoryView_FromMemory(buffer, buffersize, PyBUF_READ); if (tmp == NULL) { From c4c6bdb7a43cfd01d620c1c1864f2f0a6de738a1 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Tue, 9 Sep 2025 09:04:23 +0200 Subject: [PATCH 4/7] Ensure NanoporeReadInfo type is properly refcounted --- src/sequali/_qcmodule.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sequali/_qcmodule.c b/src/sequali/_qcmodule.c index b22516d..7306b95 100644 --- a/src/sequali/_qcmodule.c +++ b/src/sequali/_qcmodule.c @@ -4946,7 +4946,8 @@ NanoStatsIterator_FromNanoStats(NanoStats *nano_stats) if (self == NULL) { return PyErr_NoMemory(); } - self->NanoporeReadInfo_Type = state->NanoporeReadInfo_Type; + self->NanoporeReadInfo_Type = + (PyTypeObject *)Py_NewRef(state->NanoporeReadInfo_Type); self->nano_infos = nano_stats->nano_infos; self->number_of_reads = nano_stats->number_of_reads; self->current_pos = 0; From e55eebd7c299c41dbed07bf252c99fc7a8386234 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Tue, 9 Sep 2025 09:20:58 +0200 Subject: [PATCH 5/7] Use Py_NewRef rather than Py_INCREF to prevent mistakes --- src/sequali/_qcmodule.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/sequali/_qcmodule.c b/src/sequali/_qcmodule.c index 7306b95..6b69343 100644 --- a/src/sequali/_qcmodule.c +++ b/src/sequali/_qcmodule.c @@ -602,8 +602,7 @@ FastqRecordArrayView_FromPointerSizeAndObject(struct FastqMeta *records, if (records != NULL) { memcpy(self->records, records, size); } - Py_INCREF(obj); - self->obj = obj; + self->obj = Py_NewRef(obj); return (PyObject *)self; } @@ -941,16 +940,14 @@ FastqParser__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) self->read_in_size = read_in_size; self->meta_buffer = NULL; self->meta_buffer_size = 0; - Py_INCREF(file_obj); - self->file_obj = file_obj; + self->file_obj = Py_NewRef(file_obj); return (PyObject *)self; } static PyObject * FastqParser__iter__(PyObject *self) { - Py_INCREF(self); - return self; + return Py_NewRef(self); } static inline bool @@ -1491,8 +1488,7 @@ BamParser__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) self->read_in_size = read_in_size; self->meta_buffer = NULL; self->meta_buffer_size = 0; - Py_INCREF(file_obj); - self->file_obj = file_obj; + self->file_obj = Py_NewRef(file_obj); self->header = header; return (PyObject *)self; } @@ -1500,8 +1496,7 @@ BamParser__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) static PyObject * BamParser__iter__(BamParser *self) { - Py_INCREF((PyObject *)self); - return (PyObject *)self; + return Py_NewRef(self); } struct BamRecordHeader { @@ -2938,8 +2933,7 @@ AdapterCounter_get_counts(AdapterCounter *self, PyObject *Py_UNUSED(ignore)) if (counts_reverse == NULL) { return NULL; } - PyObject *adapter = PyTuple_GetItem(self->adapters, i); - Py_INCREF(adapter); + PyObject *adapter = Py_NewRef(PyTuple_GetItem(self->adapters, i)); PyObject *tup = PyTuple_New(3); PyTuple_SetItem(tup, 0, adapter); PyTuple_SetItem(tup, 1, counts_forward); @@ -4951,16 +4945,14 @@ NanoStatsIterator_FromNanoStats(NanoStats *nano_stats) self->nano_infos = nano_stats->nano_infos; self->number_of_reads = nano_stats->number_of_reads; self->current_pos = 0; - Py_INCREF((PyObject *)nano_stats); - self->nano_stats = (PyObject *)nano_stats; + self->nano_stats = Py_NewRef(nano_stats); return (PyObject *)self; } static PyObject * NanoStatsIterator__iter__(NanoStatsIterator *self) { - Py_INCREF((PyObject *)self); - return (PyObject *)self; + return Py_NewRef(self); } static PyObject * From bd9a992922b72654d86f5e71446a14bd4b487fa6 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Tue, 9 Sep 2025 10:01:29 +0200 Subject: [PATCH 6/7] use PyObject_SelfIter rather than doing it by hand --- src/sequali/_qcmodule.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/src/sequali/_qcmodule.c b/src/sequali/_qcmodule.c index 6b69343..fa3fed3 100644 --- a/src/sequali/_qcmodule.c +++ b/src/sequali/_qcmodule.c @@ -944,12 +944,6 @@ FastqParser__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) return (PyObject *)self; } -static PyObject * -FastqParser__iter__(PyObject *self) -{ - return Py_NewRef(self); -} - static inline bool buffer_contains_fastq(const uint8_t *buffer, size_t buffer_size) { @@ -1235,7 +1229,7 @@ static PyMethodDef FastqParser_methods[] = { static PyType_Slot FastqParser_slots[] = { {Py_tp_dealloc, (destructor)FastqParser_dealloc}, {Py_tp_new, FastqParser__new__}, - {Py_tp_iter, FastqParser__iter__}, + {Py_tp_iter, PyObject_SelfIter}, {Py_tp_iternext, FastqParser__next__}, {Py_tp_methods, FastqParser_methods}, {0, NULL}, @@ -1493,12 +1487,6 @@ BamParser__new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) return (PyObject *)self; } -static PyObject * -BamParser__iter__(BamParser *self) -{ - return Py_NewRef(self); -} - struct BamRecordHeader { uint32_t block_size; int32_t reference_id; @@ -1722,7 +1710,7 @@ static PyMemberDef BamParser_members[] = { static PyType_Slot BamParser_slots[] = { {Py_tp_dealloc, (destructor)BamParser_dealloc}, {Py_tp_new, BamParser__new__}, - {Py_tp_iter, BamParser__iter__}, + {Py_tp_iter, PyObject_SelfIter}, {Py_tp_iternext, BamParser__next__}, {Py_tp_members, BamParser_members}, {0, NULL}, @@ -4949,12 +4937,6 @@ NanoStatsIterator_FromNanoStats(NanoStats *nano_stats) return (PyObject *)self; } -static PyObject * -NanoStatsIterator__iter__(NanoStatsIterator *self) -{ - return Py_NewRef(self); -} - static PyObject * NanoStatsIterator__next__(NanoStatsIterator *self) { @@ -4975,7 +4957,7 @@ NanoStatsIterator__next__(NanoStatsIterator *self) static PyType_Slot NanoStatsIterator_slots[] = { {Py_tp_dealloc, (destructor)NanoStatsIterator_dealloc}, - {Py_tp_iter, (iternextfunc)NanoStatsIterator__iter__}, + {Py_tp_iter, PyObject_SelfIter}, {Py_tp_iternext, (iternextfunc)NanoStatsIterator__next__}, {0, NULL}, }; From 66afae0a72fc7526d18f48b1063c2cf2536353f5 Mon Sep 17 00:00:00 2001 From: Ruben Vorderman Date: Tue, 9 Sep 2025 11:08:49 +0200 Subject: [PATCH 7/7] Add reference counting fixes to the changelog --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6617b84..9f4a5d3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,7 @@ Changelog develop ------------------ ++ Fix various reference counting errors. + Fix issue where a missing ``st`` tag causes an IndexError. + Raise a warning on wrongly formatted ``pi`` tags rather than an error. + Fix issue where too many secondary and supplementary reads in a row would