8000 Fixes SIGSEGV and improve thread safety of journal.Reader class by sebres · Pull Request #144 · systemd/python-systemd · GitHub
[go: up one dir, main page]
Skip to content

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

New issue 8000

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should probably be dropped as well, since it is EOL or in other words: with 3.7 dropped I see no reason not to drop 3.8

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped 3.7 only because of switch to ubuntu-latest, which doesn't have it.
I don't see any reason to drop 3.8. at least as long as it is compatible and running yet.
Anyway has nothing to do with the PR as is, I made the changes just in order to repair the GHA flow.

"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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might make sense to always install setuptools, so that all versions are tested with the same?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno... IMHO, only 3.12+ missing it as package, but I am unsure the attempt to install with pip for previous versions would be always successful (and not fail due to conflict with preinstalled package).
I can imagine it could.

fi
python -m pip install pytest sphinx

8000 - 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_ 10000 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
0