Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes SIGSEGV and improve thread safety of journal.Reader class #144

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+92 −51
Open

Fixes SIGSEGV and improve thread safety of journal.Reader class #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
245784c
improve thread safety of journal.Reader class by simple protection wi…
sebres Apr 1, 2025
7fa91c8
amend: close and decrement count for initial reference only once (avo…
sebres Apr 1, 2025
7c99a39
repair GHA-CI (update versions, pip install using --break-system-pack…
sebres Apr 2, 2025
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
14 changes: 9 additions & 5 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,27 +14,28 @@ permissions:

jobs:
build:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
concurrency:
group: ${{ github.workflow }}-${{ matrix.python }}-${{ github.ref }}
cancel-in-progress: true
strategy:
fail-fast: false
matrix:
python: [
"3.7",
"3.8",
"3.9",
"3.10",
"3.11.0-rc.1",
"3.11",
"3.12",
"3.13"
]
name: Python ${{ matrix.python }}
steps:
- name: Repository checkout
uses: actions/checkout@v2
uses: actions/checkout@v4

- name: Configure Python ${{ matrix.python }}
uses: actions/setup-python@v2
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python }}
architecture: x64
Expand All @@ -43,6 +44,9 @@ jobs:
run: |
sudo apt -y update
sudo apt -y install gcc libsystemd-dev
if dpkg --compare-versions "${{ matrix.python }}" ge 3.12; then
python -m pip install setuptools || echo "can't install setuptools"
fi
python -m pip install pytest sphinx

- name: Build (Python ${{ matrix.python }})
Expand Down
19 changes: 12 additions & 7 deletions .github/workflows/install.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ jobs:
container: [
"archlinux:latest",
"debian:testing",
"quay.io/centos/centos:stream8",
"quay.io/centos/centos:stream9",
"quay.io/fedora/fedora:rawhide",
"ubuntu:focal",
]
Expand All @@ -33,7 +33,7 @@ jobs:
name: ${{ matrix.container }}
steps:
- name: Repository checkout
uses: actions/checkout@v2
uses: actions/checkout@v4

- name: Install dependencies
shell: bash
Expand All @@ -45,34 +45,39 @@ jobs:
gcc
git
pkg-config
python3
systemd
)

case "$DIST_ID" in
arch)
pacman --noconfirm -Sy python3 || echo "Issue installing/upgrading python3"
pacman --noconfirm -Sy python-pip
python3 -m pip install --upgrade pip || echo "can't upgrade pip"
pacman --noconfirm -Sy "${DEPS_COMMON[@]}" systemd-libs
python3 -m ensurepip
;;
centos|fedora)
dnf -y install "${DEPS_COMMON[@]}" systemd-devel python3-devel python3-pip
dnf -y install python3 python3-devel python3-pip
python3 -m pip install --upgrade pip || echo "can't upgrade pip"
dnf -y install "${DEPS_COMMON[@]}" systemd-devel
;;
ubuntu|debian)
apt -y update
DEBIAN_FRONTEND=noninteractive apt -y install python3 python3-dev python3-pip
python3 -m pip install --upgrade pip || echo "can't upgrade pip"
DEBIAN_FRONTEND=noninteractive apt -y install "${DEPS_COMMON[@]}" libsystemd-dev python3-dev python3-pip
;;
*)
echo >&2 "Invalid distribution ID: $DISTRO_ID"
exit 1
esac

python3 -m pip install pytest sphinx
python3 -m pip install --break-system-packages pytest sphinx

- name: Build & install
shell: bash
run: |
set -x
python3 -m pip install -I -v .
python3 -m pip install --break-system-packages -I -v .
# Avoid importing the systemd module from the git repository
cd /
python3 -c 'from systemd import journal; print(journal.__version__)'
Expand Down
110 changes: 71 additions & 39 deletions systemd/_reader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@
typedef struct {
PyObject_HEAD
sd_journal *j;
unsigned closed;
unsigned ref_count;
} Reader;
static PyTypeObject ReaderType;

Expand All @@ -89,6 +91,31 @@ static PyStructSequence_Desc Monotonic_desc = {
2,
};

static PyObject *Reader_new(PyTypeObject *type, PyObject *args, PyObject *kwds) {
Reader *self = (Reader *)PyType_GenericNew(type, args, kwds);
self->j = NULL;
self->closed = 0;
self->ref_count = 1; /* initial reference */
return (PyObject *)self;
}

static inline void decr_ref_count(Reader *self) {
if (!self->ref_count) return;
if (!--self->ref_count && self->j) {
sd_journal_close(self->j);
self->j = NULL;
}
}

#define INCR_REF_BEGIN_ALLOW_THREADS(self) \
self->ref_count++; \
Py_BEGIN_ALLOW_THREADS
#define DECR_REF_END_ALLOW_THREADS(self) \
Py_END_ALLOW_THREADS \
decr_ref_count(self);



/**
* Convert a str or bytes object into a C-string path.
* Returns NULL on error.
Expand Down Expand Up @@ -220,7 +247,10 @@ static int intlist_converter(PyObject* obj, int **_result, size_t *_len) {
}

static void Reader_dealloc(Reader* self) {
sd_journal_close(self->j);
if (self->j) {
sd_journal_close(self->j);
self->j = NULL;
}
Py_TYPE(self)->tp_free((PyObject*)self);
}

Expand Down Expand Up @@ -271,9 +301,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) {
return -1;

#if HAVE_JOURNAL_OPEN_DIRECTORY_FD
Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_open_directory_fd(&self->j, (int) fd, flags);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)
#else
r = -ENOSYS;
#endif
Expand All @@ -285,9 +315,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) {
if (!path)
return -1;

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_open_directory(&self->j, path, flags);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)
}
} else if (_files) {
_cleanup_Py_DECREF_ PyObject *item0 = NULL;
Expand All @@ -300,9 +330,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) {
return -1;

#if HAVE_JOURNAL_OPEN_FILES
Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_open_files(&self->j, (const char**) files, flags);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)
#else
r = -ENOSYS;
#endif
Expand All @@ -314,9 +344,9 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) {
return -1;

#if HAVE_JOURNAL_OPEN_DIRECTORY_FD
Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_open_files_fd(&self->j, fds, n_fds, flags);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)
#else
r = -ENOSYS;
#endif
Expand All @@ -329,16 +359,16 @@ static int Reader_init(Reader *self, PyObject *args, PyObject *keywds) {
if (!namespace)
return -1;

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_open_namespace(&self->j, namespace, flags);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)
#else
r = -ENOSYS;
#endif
} else {
Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_open(&self->j, flags);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)
}

return set_error(r, NULL, "Opening the journal failed");
Expand Down Expand Up @@ -438,8 +468,10 @@ static PyObject* Reader_close(Reader *self, PyObject *args) {
assert(self);
assert(!args);

sd_journal_close(self->j);
self->j = NULL;
if (!self->closed) {
self->closed = 1;
decr_ref_count(self); /* decrement initial reference (without incr) */
}
Py_RETURN_NONE;
}

Expand Down Expand Up @@ -501,7 +533,7 @@ static PyObject* Reader_next(Reader *self, PyObject *args) {
return NULL;
}

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
if (skip == 1LL)
r = sd_journal_next(self->j);
else if (skip == -1LL)
Expand All @@ -512,7 +544,7 @@ static PyObject* Reader_next(Reader *self, PyObject *args) {
r = sd_journal_previous_skip(self->j, -skip);
else
assert(!"should be here");
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, NULL) < 0)
return NULL;
Expand Down Expand Up @@ -782,9 +814,9 @@ PyDoc_STRVAR(Reader_seek_head__doc__,
"See :manpage:`sd_journal_seek_head(3)`.");
static PyObject* Reader_seek_head(Reader *self, PyObject *args) {
int r;
Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_seek_head(self->j);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, NULL) < 0)
return NULL;
Expand All @@ -800,9 +832,9 @@ PyDoc_STRVAR(Reader_seek_tail__doc__,
static PyObject* Reader_seek_tail(Reader *self, PyObject *args) {
int r;

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_seek_tail(self->j);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, NULL) < 0)
return NULL;
Expand All @@ -820,9 +852,9 @@ static PyObject* Reader_seek_realtime(Reader *self, PyObject *args) {
if (!PyArg_ParseTuple(args, "K:seek_realtime", &timestamp))
return NULL;

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_seek_realtime_usec(self->j, timestamp);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, NULL) < 0)
return NULL;
Expand Down Expand Up @@ -850,17 +882,17 @@ static PyObject* Reader_seek_monotonic(Reader *self, PyObject *args) {
if (set_error(r, NULL, "Invalid bootid") < 0)
return NULL;
} else {
Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_id128_get_boot(&id);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, NULL) < 0)
return NULL;
}

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_seek_monotonic_usec(self->j, id, timestamp);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, NULL) < 0)
return NULL;
Expand Down Expand Up @@ -924,9 +956,9 @@ static PyObject* Reader_process(Reader *self, PyObject *args) {

assert(!args);

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_process(self->j);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)
if (set_error(r, NULL, NULL) < 0)
return NULL;

Expand All @@ -950,9 +982,9 @@ static PyObject* Reader_wait(Reader *self, PyObject *args) {
if (!PyArg_ParseTuple(args, "|L:wait", &timeout))
return NULL;

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_wait(self->j, timeout);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, NULL) < 0)
return NULL;
Expand All @@ -970,9 +1002,9 @@ static PyObject* Reader_seek_cursor(Reader *self, PyObject *args) {
if (!PyArg_ParseTuple(args, "s:seek_cursor", &cursor))
return NULL;

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_seek_cursor(self->j, cursor);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, "Invalid cursor") < 0)
return NULL;
Expand Down Expand Up @@ -1035,9 +1067,9 @@ static PyObject* Reader_query_unique(Reader *self, PyObject *args) {
if (!PyArg_ParseTuple(args, "s:query_unique", &query))
return NULL;

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_query_unique(self->j, query);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (set_error(r, NULL, "Invalid field name") < 0)
return NULL;
Expand Down Expand Up @@ -1172,9 +1204,9 @@ static PyObject* Reader_get_catalog(Reader *self, PyObject *args) {
assert(self);
assert(!args);

Py_BEGIN_ALLOW_THREADS
INCR_REF_BEGIN_ALLOW_THREADS(self)
r = sd_journal_get_catalog(self->j, &msg);
Py_END_ALLOW_THREADS
DECR_REF_END_ALLOW_THREADS(self)

if (r == -ENOENT) {
const void* mid;
Expand Down Expand Up @@ -1264,7 +1296,7 @@ static int Reader_set_data_threshold(Reader *self, PyObject *value, void *closur
PyDoc_STRVAR(closed__doc__,
"True iff journal is closed");
static PyObject* Reader_get_closed(Reader *self, void *closure) {
return PyBool_FromLong(!self->j);
return PyBool_FromLong(self->closed || !self->j);
}

static PyGetSetDef Reader_getsetters[] = {
Expand Down Expand Up @@ -1330,7 +1362,7 @@ static PyTypeObject ReaderType = {
.tp_methods = Reader_methods,
.tp_getset = Reader_getsetters,
.tp_init = (initproc) Reader_init,
.tp_new = PyType_GenericNew,
.tp_new = Reader_new,
};

static PyMethodDef methods[] = {
Expand Down
Loading