src/util/virrotatingfile.c | 49 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 15 deletions(-)
Truncate means that if a file exists it's length will be truncated to
0, but the mode and the owner shall be unchanged. The current behavior
is that the original file is deleted and a new file is created. Let's
fix this by using O_TRUNC.
The function virRotatingFileWriterDelete is now unused but may be used
in the future and is therefore still defined.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
Note:
This change has the (potentially unwanted) security effect that the
owner/group of the log file does not change. Before this patch the old
log file was deleted and the newly created log file was owned by the
virtlogd user. Now, if a user has created the log file before, he can
read the logs. If we don't wanna have this effect we can either
adjust/add a virtlogd API or do a chown within the calling driver
(e.g. QEMU driver).
Side note: the original behavior has changed with the patch series
"qemu: use FD passing for chardev UNIX sockets".
Example where it the behavior has changed:
<console type='file'>
<source path='/tmp/console.log'/>
<target type='serial'/>
</console>
---
src/util/virrotatingfile.c | 49 ++++++++++++++++++++++++++++++++--------------
1 file changed, 34 insertions(+), 15 deletions(-)
diff --git a/src/util/virrotatingfile.c b/src/util/virrotatingfile.c
index ca62a8e02641..d6ab3780aae3 100644
--- a/src/util/virrotatingfile.c
+++ b/src/util/virrotatingfile.c
@@ -98,17 +98,25 @@ virRotatingFileReaderEntryFree(virRotatingFileReaderEntryPtr entry)
static virRotatingFileWriterEntryPtr
virRotatingFileWriterEntryNew(const char *path,
- mode_t mode)
+ mode_t mode,
+ bool truncate)
{
virRotatingFileWriterEntryPtr entry;
struct stat sb;
+ /* O_APPEND is also useful in combination with O_TRUNC since it
+ * guarantees the atomicity of a write operation (at least for
+ * POSIX systems) */
+ int oflag = O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC;
VIR_DEBUG("Opening %s mode=0%02o", path, mode);
if (VIR_ALLOC(entry) < 0)
return NULL;
- if ((entry->fd = open(path, O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC, mode)) < 0) {
+ if (truncate)
+ oflag |= O_TRUNC;
+
+ if ((entry->fd = open(path, oflag, mode)) < 0) {
virReportSystemError(errno,
_("Unable to open file: %s"), path);
goto error;
@@ -182,18 +190,10 @@ virRotatingFileReaderEntryNew(const char *path)
static int
-virRotatingFileWriterDelete(virRotatingFileWriterPtr file)
+virRotatingFileWriterDeleteBackup(virRotatingFileWriterPtr file)
{
size_t i;
- if (unlink(file->basepath) < 0 &&
- errno != ENOENT) {
- virReportSystemError(errno,
- _("Unable to delete file %s"),
- file->basepath);
- return -1;
- }
-
for (i = 0; i < file->maxbackup; i++) {
char *oldpath;
if (virAsprintf(&oldpath, "%s.%zu", file->basepath, i) < 0)
@@ -214,6 +214,24 @@ virRotatingFileWriterDelete(virRotatingFileWriterPtr file)
}
+static int ATTRIBUTE_UNUSED
+virRotatingFileWriterDelete(virRotatingFileWriterPtr file)
+{
+ if (unlink(file->basepath) < 0 &&
+ errno != ENOENT) {
+ virReportSystemError(errno,
+ _("Unable to delete file %s"),
+ file->basepath);
+ return -1;
+ }
+
+ if (virRotatingFileWriterDeleteBackup(file) < 0)
+ return -1;
+
+ return 0;
+}
+
+
/**
* virRotatingFileWriterNew
* @path: the base path for files
@@ -257,12 +275,12 @@ virRotatingFileWriterNew(const char *path,
file->maxbackup = maxbackup;
file->maxlen = maxlen;
- if (trunc &&
- virRotatingFileWriterDelete(file) < 0)
+ if (trunc && virRotatingFileWriterDeleteBackup(file) < 0)
goto error;
if (!(file->entry = virRotatingFileWriterEntryNew(file->basepath,
- mode)))
+ mode,
+ trunc)))
goto error;
return file;
@@ -491,7 +509,8 @@ virRotatingFileWriterAppend(virRotatingFileWriterPtr file,
return -1;
if (!(tmp = virRotatingFileWriterEntryNew(file->basepath,
- file->mode)))
+ file->mode,
+ false)))
return -1;
virRotatingFileWriterEntryFree(file->entry);
--
2.13.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Aug 21, 2018 at 10:49:28AM +0200, Marc Hartmayer wrote: > Truncate means that if a file exists it's length will be truncated to > 0, but the mode and the owner shall be unchanged. The current behavior > is that the original file is deleted and a new file is created. Let's > fix this by using O_TRUNC. This is just describing what you've changed, leaving out why you are trying todo this ? > The function virRotatingFileWriterDelete is now unused but may be used > in the future and is therefore still defined. > > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> > --- > Note: > > This change has the (potentially unwanted) security effect that the > owner/group of the log file does not change. Before this patch the old > log file was deleted and the newly created log file was owned by the > virtlogd user. Now, if a user has created the log file before, he can > read the logs. If we don't wanna have this effect we can either > adjust/add a virtlogd API or do a chown within the calling driver > (e.g. QEMU driver). Pre-creating the log file and/or messing around with ownership are not things we ever intended to support. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Aug 21, 2018 at 11:03 AM +0200, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
> On Tue, Aug 21, 2018 at 10:49:28AM +0200, Marc Hartmayer wrote:
>> Truncate means that if a file exists it's length will be truncated to
>> 0, but the mode and the owner shall be unchanged. The current behavior
>> is that the original file is deleted and a new file is created. Let's
>> fix this by using O_TRUNC.
>
> This is just describing what you've changed, leaving out why you are
> trying todo this ?
Two things:
1. The behavior for a console that logs all data to a file has changed
with the patch series “qemu: use FD passing for chardev UNIX
sockets”. Before this patch series the owner of the log file was the
QEMU process user (since the QEMU process was responsible for
creating it). Now it’s the virtlogd user.
e.g.
<console type='file'>
<source path='/tmp/console.log'/>
<target type='serial'/>
</console>
2. What is currently done by virtlogd is not a truncation (at least
IMHO). An alternative to this patch would be to rename the parameter
(API change… so probably no option) or document the behavior.
>
>> The function virRotatingFileWriterDelete is now unused but may be used
>> in the future and is therefore still defined.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>> Note:
>>
>> This change has the (potentially unwanted) security effect that the
>> owner/group of the log file does not change. Before this patch the old
>> log file was deleted and the newly created log file was owned by the
>> virtlogd user. Now, if a user has created the log file before, he can
>> read the logs. If we don't wanna have this effect we can either
>> adjust/add a virtlogd API or do a chown within the calling driver
>> (e.g. QEMU driver).
>
> Pre-creating the log file and/or messing around with ownership are
> not things we ever intended to support.
Okay. What should happen if the log file is already pre-created?
Silently overwrite/delete?
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Aug 21, 2018 at 12:37:54PM +0200, Marc Hartmayer wrote: > On Tue, Aug 21, 2018 at 11:03 AM +0200, "Daniel P. Berrangé" <berrange@redhat.com> wrote: > > On Tue, Aug 21, 2018 at 10:49:28AM +0200, Marc Hartmayer wrote: > >> Truncate means that if a file exists it's length will be truncated to > >> 0, but the mode and the owner shall be unchanged. The current behavior > >> is that the original file is deleted and a new file is created. Let's > >> fix this by using O_TRUNC. > > > > This is just describing what you've changed, leaving out why you are > > trying todo this ? > > Two things: > > 1. The behavior for a console that logs all data to a file has changed > with the patch series “qemu: use FD passing for chardev UNIX > sockets”. Before this patch series the owner of the log file was the > QEMU process user (since the QEMU process was responsible for > creating it). Now it’s the virtlogd user. > > e.g. > > <console type='file'> > <source path='/tmp/console.log'/> > <target type='serial'/> > </console> Yes, that ownership change is *good* because the point of the change was to prevent the QEMU process from ever writing to the logs directly, so that virtlogd can enforce rollover policies. > 2. What is currently done by virtlogd is not a truncation (at least > IMHO). An alternative to this patch would be to rename the parameter > (API change… so probably no option) or document the behavior. It depends on your POV. If you are looking at the inode it isn't truncation, because we've just given the inode a new name, and created a new inode for the original name. If you are looking at the filename this is truncation, because the file "foo.log" used to have a size and now it is zero length. > >> The function virRotatingFileWriterDelete is now unused but may be used > >> in the future and is therefore still defined. > >> > >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > >> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> > >> --- > >> Note: > >> > >> This change has the (potentially unwanted) security effect that the > >> owner/group of the log file does not change. Before this patch the old > >> log file was deleted and the newly created log file was owned by the > >> virtlogd user. Now, if a user has created the log file before, he can > >> read the logs. If we don't wanna have this effect we can either > >> adjust/add a virtlogd API or do a chown within the calling driver > >> (e.g. QEMU driver). > > > > Pre-creating the log file and/or messing around with ownership are > > not things we ever intended to support. > > Okay. What should happen if the log file is already pre-created? > Silently overwrite/delete? It'll just have the same behaviour is if the file already existed from a previous boot attempt of the VM. Normal rollover should occurr when the file size hits the limit. Any changes to ownership are liable to be thrown away. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.