Re: [systemd-devel] [Fwd: [PATCH] journal: fix dereferenced pointer in journal_file_rotate()]
On Tue, 2012-05-22 at 08:40 +0200, Sjoerd Simons wrote:
On Mon, 2012-05-21 at 21:35 -0700, shawn wrote:
If journal_file_open() failed, due to (e.g.) -ENOSPC on open()
new_file might still be NULL.
On error, leave pointer to the old JournalFile (now closed),
and require caller to check for error approiately.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43020
Reported-by: Sjoerd Simons sjo...@luon.net
The bugzilla link seems wrong ?
yes, I was looking at your patch, (via debian BTS) but I copied the url
wrong. (corrected patch attached) That is how I knew to forward it to
you. I read your patch, however there are many reasons other than ENOSPC
why open() could fail leaving a null pointer, and my patch takes care of
that.
This actually remind me though, i did submit a patch for this issue to
bugzilla (slightly different then your solution) more then a month ago.
And a companion patch to not make the issue occur so easily, bugs filed
here:
https://bugs.freedesktop.org/show_bug.cgi?id=48688
https://bugs.freedesktop.org/show_bug.cgi?id=48685
If the systemd bugzilla is just somewhat of a decoy i'm happy to repost
the patches to the list ofcourse :)
Well the first patch I submitted to systemd bugzilla sure didn't get any
traction
--
-Shawn Landden
From 54a970dcf59b59ade587002925be027b71d81545 Mon Sep 17 00:00:00 2001
From: Shawn Landden shawnland...@gmail.com
Date: Mon, 21 May 2012 19:46:54 -0700
Subject: [PATCH] journal: fix dereferenced pointer in journal_file_rotate()
If journal_file_open() failed, due to (e.g.) -ENOSPC on open()
new_file might still be NULL.
On error, leave pointer to the old JournalFile (now closed),
and require caller to check for error approiately.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48685
Reported-by: Sjoerd Simons sjo...@luon.net
---
src/journal/journal-file.c |9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
index 5dd6e57..9f5f26e 100644
--- a/src/journal/journal-file.c
+++ b/src/journal/journal-file.c
@@ -1871,9 +1871,16 @@ int journal_file_rotate(JournalFile **f) {
old_file-header-state = STATE_ARCHIVED;
r = journal_file_open(old_file-path, old_file-flags, old_file-mode, old_file, new_file);
-journal_file_close(old_file);
+
+if (r 0) {
+r = -errno;
+goto finish;
+}
*f = new_file;
+
+finish:
+journal_file_close(old_file);
return r;
}
--
1.7.9.5
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [Fwd: [PATCH] journal: fix dereferenced pointer in journal_file_rotate()]
On Mon, 2012-05-21 at 21:35 -0700, shawn wrote:
If journal_file_open() failed, due to (e.g.) -ENOSPC on open()
new_file might still be NULL.
On error, leave pointer to the old JournalFile (now closed),
and require caller to check for error approiately.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43020
Reported-by: Sjoerd Simons sjo...@luon.net
The bugzilla link seems wrong ?
This actually remind me though, i did submit a patch for this issue to
bugzilla (slightly different then your solution) more then a month ago.
And a companion patch to not make the issue occur so easily, bugs filed
here:
https://bugs.freedesktop.org/show_bug.cgi?id=48688
https://bugs.freedesktop.org/show_bug.cgi?id=48685
If the systemd bugzilla is just somewhat of a decoy i'm happy to repost
the patches to the list ofcourse :)
src/journal/journal-file.c |9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
index 5dd6e57..9f5f26e 100644
--- a/src/journal/journal-file.c
+++ b/src/journal/journal-file.c
@@ -1871,9 +1871,16 @@ int journal_file_rotate(JournalFile **f) {
old_file-header-state = STATE_ARCHIVED;
r = journal_file_open(old_file-path, old_file-flags,
old_file-mode, old_file, new_file);
-journal_file_close(old_file);
+
+if (r 0) {
+r = -errno;
+goto finish;
+}
*f = new_file;
+
+finish:
+journal_file_close(old_file);
return r;
}
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [Fwd: [PATCH] journal: fix dereferenced pointer in journal_file_rotate()]
On Tue, 22.05.12 08:40, Sjoerd Simons (sjo...@luon.net) wrote: On Mon, 2012-05-21 at 21:35 -0700, shawn wrote: If journal_file_open() failed, due to (e.g.) -ENOSPC on open() new_file might still be NULL. On error, leave pointer to the old JournalFile (now closed), and require caller to check for error approiately. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43020 Reported-by: Sjoerd Simons sjo...@luon.net The bugzilla link seems wrong ? This actually remind me though, i did submit a patch for this issue to bugzilla (slightly different then your solution) more then a month ago. And a companion patch to not make the issue occur so easily, bugs filed here: https://bugs.freedesktop.org/show_bug.cgi?id=48688 https://bugs.freedesktop.org/show_bug.cgi?id=48685 If the systemd bugzilla is just somewhat of a decoy i'm happy to repost the patches to the list ofcourse :) Nah, fdo bz is not a decoy. The reason I didnt merge this right away was actually that I wanted to rework the code in question in a bigger way, so that we have some logic in there that we automatically fallback to kmsg logging when the journal for some reason doesn't work. But I never found the time to. Anyway, since this is a bug I have now merged your patch 48685, and we can add the kmsg fallback logic later on. Thanks for your work! About 48688 I am not sure sure. i.e. should we really bind the keep_free stuff to the reserved percentage of the FS? They are two different things, or are they not? Thanks! Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [Fwd: [PATCH] journal: fix dereferenced pointer in journal_file_rotate()]
On Tue, 2012-05-22 at 13:26 +0200, Lennart Poettering wrote: On Tue, 22.05.12 08:40, Sjoerd Simons (sjo...@luon.net) wrote: On Mon, 2012-05-21 at 21:35 -0700, shawn wrote: If journal_file_open() failed, due to (e.g.) -ENOSPC on open() new_file might still be NULL. On error, leave pointer to the old JournalFile (now closed), and require caller to check for error approiately. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43020 Reported-by: Sjoerd Simons sjo...@luon.net The bugzilla link seems wrong ? This actually remind me though, i did submit a patch for this issue to bugzilla (slightly different then your solution) more then a month ago. And a companion patch to not make the issue occur so easily, bugs filed here: https://bugs.freedesktop.org/show_bug.cgi?id=48688 https://bugs.freedesktop.org/show_bug.cgi?id=48685 If the systemd bugzilla is just somewhat of a decoy i'm happy to repost the patches to the list ofcourse :) Nah, fdo bz is not a decoy. The reason I didnt merge this right away was actually that I wanted to rework the code in question in a bigger way, so that we have some logic in there that we automatically fallback to kmsg logging when the journal for some reason doesn't work. But I never found the time to. Aha! A note on the bug would have been great :). Anyway, since this is a bug I have now merged your patch 48685, and we can add the kmsg fallback logic later on. Thanks for your work! Np, thanks for merging :). About 48688 I am not sure sure. i.e. should we really bind the keep_free stuff to the reserved percentage of the FS? They are two different things, or are they not? I'm not sure either as i mentioned in the bugreport. The standard 5% is a bit odd though. Although I may be a bit odd, but my / partition tends to never have a lot of free space, which means it's basically always below the 5% (Which is still more then 0.5G on a 10G system..). I picked up using the reserved precentage mostly as it's the one place i could think of where there is currently a configuration for leave this much space free please. Furthermore it means that you get a no space error only when df shows you have 0 available space which is nice i'd think :).. It took me quite some time to figure out why journald was giving out of space errors while df was happily showing there was still quite a bit of space left. -- Sjoerd Simons sjo...@luon.net ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
