Skip to content

Commit c744ef4

Browse files
committed
Fix memory leaks, deadlock risk, and integer overflow in PyBuffer
Memory leaks: - Py_BuildValue("()") in readlines and iternext was never DECREF'd - Now uses PyTuple_New(0) with proper cleanup Deadlock prevention: - read/readline/read_nonblock now copy data while holding mutex, then release mutex before calling Python APIs (PyBytes_FromStringAndSize) - This avoids holding mutex while reacquiring GIL, which could deadlock with other functions (find, readable_amount, etc.) that hold GIL while acquiring mutex Integer overflow: - Added SIZE_MAX check before computing required capacity in py_buffer_write
1 parent cad49b1 commit c744ef4

1 file changed

Lines changed: 114 additions & 31 deletions

File tree

c_src/py_buffer.c

Lines changed: 114 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@
3737
#include "py_buffer.h"
3838
#include <string.h>
3939
#include <errno.h>
40+
#include <stdint.h>
41+
#include <stdlib.h>
4042

4143
/* Portable memmem fallback for systems that don't have it.
4244
* memmem is available on: Linux (glibc), macOS, FreeBSD >= 13
@@ -141,6 +143,12 @@ int py_buffer_write(py_buffer_resource_t *buf, const unsigned char *data, size_t
141143
return -1;
142144
}
143145

146+
/* Check for overflow */
147+
if (size > SIZE_MAX - buf->write_pos) {
148+
pthread_mutex_unlock(&buf->mutex);
149+
return -1;
150+
}
151+
144152
/* Check if we need to grow the buffer */
145153
size_t required = buf->write_pos + size;
146154
if (required > buf->capacity) {
@@ -310,28 +318,48 @@ static PyObject *PyBuffer_read(PyBufferObject *self, PyObject *args) {
310318
/* Read all available (or wait for EOF if not all data yet) */
311319
if (!buf->eof && buf->content_length > 0 &&
312320
buf->write_pos < (size_t)buf->content_length) {
313-
/* Wait for all data */
321+
/* Wait for all data - release mutex before reacquiring GIL to avoid deadlock */
322+
pthread_mutex_unlock(&buf->mutex);
323+
314324
Py_BEGIN_ALLOW_THREADS
325+
pthread_mutex_lock(&buf->mutex);
315326
while (buf->write_pos < (size_t)buf->content_length &&
316327
!buf->eof && !buf->closed) {
317328
pthread_cond_wait(&buf->data_ready, &buf->mutex);
318329
}
319330
Py_END_ALLOW_THREADS
331+
320332
available = buf->write_pos - buf->read_pos;
321333
}
322334
to_read = available;
323335
} else {
324336
to_read = (available < (size_t)size) ? available : (size_t)size;
325337
}
326338

327-
/* Zero-copy: create bytes directly from buffer data */
328-
PyObject *result = PyBytes_FromStringAndSize(
329-
(char *)buf->data + buf->read_pos, to_read);
330-
331-
buf->read_pos += to_read;
339+
/* Copy data while holding mutex, then release before creating PyBytes */
340+
unsigned char *local_copy = NULL;
341+
if (to_read > 0) {
342+
local_copy = (unsigned char *)malloc(to_read);
343+
if (local_copy == NULL) {
344+
pthread_mutex_unlock(&buf->mutex);
345+
PyErr_NoMemory();
346+
return NULL;
347+
}
348+
memcpy(local_copy, buf->data + buf->read_pos, to_read);
349+
buf->read_pos += to_read;
350+
}
332351

333352
pthread_mutex_unlock(&buf->mutex);
334353

354+
/* Create PyBytes without holding mutex */
355+
PyObject *result;
356+
if (to_read > 0) {
357+
result = PyBytes_FromStringAndSize((char *)local_copy, to_read);
358+
free(local_copy);
359+
} else {
360+
result = PyBytes_FromStringAndSize("", 0);
361+
}
362+
335363
return result;
336364
}
337365

@@ -352,28 +380,46 @@ static PyObject *PyBuffer_read_nonblock(PyBufferObject *self, PyObject *args) {
352380
}
353381

354382
py_buffer_resource_t *buf = self->resource;
383+
unsigned char *local_copy = NULL;
384+
size_t to_read = 0;
355385

356386
pthread_mutex_lock(&buf->mutex);
357387

358388
/* Calculate available data */
359389
size_t available = buf->write_pos - buf->read_pos;
360390

361391
/* Determine how much to read */
362-
size_t to_read;
363392
if (size < 0) {
364393
to_read = available;
365394
} else {
366395
to_read = (available < (size_t)size) ? available : (size_t)size;
367396
}
368397

369-
/* Create result bytes */
370-
PyObject *result = PyBytes_FromStringAndSize(
371-
(char *)buf->data + buf->read_pos, to_read);
372-
373-
buf->read_pos += to_read;
398+
/* Copy data while holding mutex */
399+
if (to_read > 0) {
400+
local_copy = (unsigned char *)malloc(to_read);
401+
if (local_copy != NULL) {
402+
memcpy(local_copy, buf->data + buf->read_pos, to_read);
403+
buf->read_pos += to_read;
404+
}
405+
}
374406

375407
pthread_mutex_unlock(&buf->mutex);
376408

409+
/* Create PyBytes without holding mutex */
410+
if (local_copy == NULL && to_read > 0) {
411+
PyErr_NoMemory();
412+
return NULL;
413+
}
414+
415+
PyObject *result;
416+
if (to_read > 0) {
417+
result = PyBytes_FromStringAndSize((char *)local_copy, to_read);
418+
free(local_copy);
419+
} else {
420+
result = PyBytes_FromStringAndSize("", 0);
421+
}
422+
377423
return result;
378424
}
379425

@@ -430,10 +476,11 @@ static PyObject *PyBuffer_readline(PyBufferObject *self, PyObject *args) {
430476
}
431477

432478
py_buffer_resource_t *buf = self->resource;
479+
unsigned char *local_copy = NULL;
480+
size_t copy_len = 0;
433481

434482
Py_BEGIN_ALLOW_THREADS
435483
pthread_mutex_lock(&buf->mutex);
436-
Py_END_ALLOW_THREADS
437484

438485
/* Search for newline in available data */
439486
while (true) {
@@ -452,34 +499,54 @@ static PyObject *PyBuffer_readline(PyBufferObject *self, PyObject *args) {
452499
const unsigned char *newline = find_newline(start, search_len);
453500

454501
if (newline != NULL) {
455-
/* Found newline - return line including newline */
456-
size_t line_len = (newline - start) + 1;
457-
PyObject *result = PyBytes_FromStringAndSize((char *)start, line_len);
458-
buf->read_pos += line_len;
459-
pthread_mutex_unlock(&buf->mutex);
460-
return result;
502+
/* Found newline - copy line including newline */
503+
copy_len = (newline - start) + 1;
504+
local_copy = (unsigned char *)malloc(copy_len);
505+
if (local_copy != NULL) {
506+
memcpy(local_copy, start, copy_len);
507+
buf->read_pos += copy_len;
508+
}
509+
break;
461510
}
462511

463512
/* No newline found - check if we should return what we have */
464513
if (buf->eof || (size > 0 && available >= (size_t)size)) {
465-
size_t to_read = (size > 0 && (size_t)size < available)
466-
? (size_t)size : available;
467-
PyObject *result = PyBytes_FromStringAndSize((char *)start, to_read);
468-
buf->read_pos += to_read;
469-
pthread_mutex_unlock(&buf->mutex);
470-
return result;
514+
copy_len = (size > 0 && (size_t)size < available)
515+
? (size_t)size : available;
516+
local_copy = (unsigned char *)malloc(copy_len);
517+
if (local_copy != NULL) {
518+
memcpy(local_copy, start, copy_len);
519+
buf->read_pos += copy_len;
520+
}
521+
break;
471522
}
472523
} else if (buf->eof || buf->closed) {
473524
/* No more data coming */
474-
pthread_mutex_unlock(&buf->mutex);
475-
return PyBytes_FromStringAndSize("", 0);
525+
break;
476526
}
477527

478528
/* Wait for more data */
479-
Py_BEGIN_ALLOW_THREADS
480529
pthread_cond_wait(&buf->data_ready, &buf->mutex);
481-
Py_END_ALLOW_THREADS
482530
}
531+
532+
pthread_mutex_unlock(&buf->mutex);
533+
Py_END_ALLOW_THREADS
534+
535+
/* Create PyBytes without holding mutex */
536+
if (local_copy == NULL && copy_len > 0) {
537+
PyErr_NoMemory();
538+
return NULL;
539+
}
540+
541+
PyObject *result;
542+
if (copy_len > 0) {
543+
result = PyBytes_FromStringAndSize((char *)local_copy, copy_len);
544+
free(local_copy);
545+
} else {
546+
result = PyBytes_FromStringAndSize("", 0);
547+
}
548+
549+
return result;
483550
}
484551

485552
/**
@@ -496,9 +563,16 @@ static PyObject *PyBuffer_readlines(PyBufferObject *self, PyObject *args) {
496563

497564
Py_ssize_t total_size = 0;
498565

566+
PyObject *empty_args = PyTuple_New(0);
567+
if (empty_args == NULL) {
568+
Py_DECREF(lines);
569+
return NULL;
570+
}
571+
499572
while (true) {
500-
PyObject *line = PyBuffer_readline(self, Py_BuildValue("()"));
573+
PyObject *line = PyBuffer_readline(self, empty_args);
501574
if (line == NULL) {
575+
Py_DECREF(empty_args);
502576
Py_DECREF(lines);
503577
return NULL;
504578
}
@@ -511,6 +585,7 @@ static PyObject *PyBuffer_readlines(PyBufferObject *self, PyObject *args) {
511585

512586
if (PyList_Append(lines, line) < 0) {
513587
Py_DECREF(line);
588+
Py_DECREF(empty_args);
514589
Py_DECREF(lines);
515590
return NULL;
516591
}
@@ -524,6 +599,7 @@ static PyObject *PyBuffer_readlines(PyBufferObject *self, PyObject *args) {
524599
}
525600
}
526601

602+
Py_DECREF(empty_args);
527603
return lines;
528604
}
529605

@@ -710,7 +786,14 @@ static PyObject *PyBuffer_iter(PyObject *self) {
710786
}
711787

712788
static PyObject *PyBuffer_iternext(PyBufferObject *self) {
713-
PyObject *line = PyBuffer_readline(self, Py_BuildValue("()"));
789+
PyObject *empty_args = PyTuple_New(0);
790+
if (empty_args == NULL) {
791+
return NULL;
792+
}
793+
794+
PyObject *line = PyBuffer_readline(self, empty_args);
795+
Py_DECREF(empty_args);
796+
714797
if (line == NULL) {
715798
return NULL;
716799
}

0 commit comments

Comments
 (0)