noxdafox
2015-Nov-08 11:43 UTC
Re: [libvirt-users] virDomainCoreDumpWithFormat files created as root
Michal Privoznik
2015-Nov-18 10:11 UTC
Re: [libvirt-users] virDomainCoreDumpWithFormat files created as root
On 08.11.2015 12:43, noxdafox wrote:> I've been spending a bit of time looking into libvirt's code and I > believe this is not implemented as Daniel first said. > > The issue is in the qemuOpenFileAs function in src/qemu/qemu_driver.c > which ignores the dynamic ownership flag and does not set correct > ownership on the file. > > The qemuOpenFileAs function is used by other ones as well, so I wonder > if this affects other QEMU features. > > I tried to fix and test it in different use cases and I didn't notice > any side effect, yet I'd like to provide tests as this function is quite > a core one and it's implementation is a bit confusing (maybe a > refactoring would simplify its logic). > Unfortunately the function is not exposed, therefore unittests are a bit > challenging. Higher level tests seem to mock the driver > (src/test/test_driver.c) as well. > > Here's the patch fixing the issue. I set the correct uid and gid only if > the file is being created and dynamic ownership is set. > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index a2cc002..1b47dc6 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -2932,6 +2932,11 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t > fallback_gid, > if (path_shared <= 0 || dynamicOwnership) > vfoflags |= VIR_FILE_OPEN_FORCE_OWNER; > > + if (dynamicOwnership) { > + uid = fallback_uid; > + gid = fallback_gid; > + } > + > if (stat(path, &sb) == 0) { > /* It already exists, we don't want to delete it on error */ > need_unlink = false; > > Please let me know what do you think about it and if we can somehow > include it in the sources. I'd be glad to offer further help if necessary.Can you send it as an actual patch? Looks reasonable to me. Michal
noxdafox
2015-Nov-18 18:23 UTC
Re: [libvirt-users] virDomainCoreDumpWithFormat files created as root
On 18/11/15 12:11, Michal Privoznik wrote:> On 08.11.2015 12:43, noxdafox wrote: >> I've been spending a bit of time looking into libvirt's code and I >> believe this is not implemented as Daniel first said. >> >> The issue is in the qemuOpenFileAs function in src/qemu/qemu_driver.c >> which ignores the dynamic ownership flag and does not set correct >> ownership on the file. >> >> The qemuOpenFileAs function is used by other ones as well, so I wonder >> if this affects other QEMU features. >> >> I tried to fix and test it in different use cases and I didn't notice >> any side effect, yet I'd like to provide tests as this function is quite >> a core one and it's implementation is a bit confusing (maybe a >> refactoring would simplify its logic). >> Unfortunately the function is not exposed, therefore unittests are a bit >> challenging. Higher level tests seem to mock the driver >> (src/test/test_driver.c) as well. >> >> Here's the patch fixing the issue. I set the correct uid and gid only if >> the file is being created and dynamic ownership is set. >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index a2cc002..1b47dc6 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -2932,6 +2932,11 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t >> fallback_gid, >> if (path_shared <= 0 || dynamicOwnership) >> vfoflags |= VIR_FILE_OPEN_FORCE_OWNER; >> >> + if (dynamicOwnership) { >> + uid = fallback_uid; >> + gid = fallback_gid; >> + } >> + >> if (stat(path, &sb) == 0) { >> /* It already exists, we don't want to delete it on error */ >> need_unlink = false; >> >> Please let me know what do you think about it and if we can somehow >> include it in the sources. I'd be glad to offer further help if necessary. > Can you send it as an actual patch? Looks reasonable to me. > > MichalDone, please let me know if I did something wrong as it's the first patch I'm sending. BR.