[PATCH] rpm: Avoid leaking temporary scriplet files

[PATCH] rpm: Avoid leaking temporary scriplet files

[Olof Johansson]

RPM writes each package scriptlet (post-/preinstall) to /var/tmp/rpm-tmp.XXXXXX
 --- a lot of files get created. When debugging is enabled, these temporary
scriptlet files aren't cleaned up at all and after a while this results in the
filesystem resources are eaten up (like running out of available inodes).

This is a quick fix to avoid this. It does degrade functionality for
those working with debugging pre-/postintall scripts.

Fixes [YOCTO #12792]

Signed-off-by: Olof Johansson <olofjn@axis.com>
---
 ...ve-written-tmp-scriptlets-even-with-debug.patch | 37 ++++++++++++++++++++++
 meta/recipes-devtools/rpm/rpm_4.14.1.bb            |  6 ++++
 2 files changed, 43 insertions(+)
 create mode 100644 meta/recipes-devtools/rpm/files/0001-Always-remove-written-tmp-scriptlets-even-with-debug.patch

diff --git a/meta/recipes-devtools/rpm/files/0001-Always-remove-written-tmp-scriptlets-even-with-debug.patch b/meta/recipes-devtools/rpm/files/0001-Always-remove-written-tmp-scriptlets-even-with-debug.patch
new file mode 100644
index 00000000000..fe4b45bfb3b
--- /dev/null
+++ b/meta/recipes-devtools/rpm/files/0001-Always-remove-written-tmp-scriptlets-even-with-debug.patch
@@ -0,0 +1,37 @@
+From b74b549ff61ab8a2bdb041febbe135ad10fa303c Mon Sep 17 00:00:00 2001
+From: Olof Johansson <olofjn@axis.com>
+Date: Sun, 17 Jun 2018 12:12:36 +0200
+Subject: [PATCH] Always remove written tmp scriptlets, even with debugging
+ enabled
+
+RPM writes each package scriptlet (post-/preinstall) to /var/tmp/rpm-tmp.XXXXXX
+ --- a lot of files get created. When debugging is enabled, these temporary
+scriptlet files aren't cleaned up at all and after a while this results in the
+filesystem resources are eaten up (like running out of available inodes).
+
+This is a quick fix to avoid this. It does degrade functionality for
+those working with debugging pre-/postintall scripts.
+
+Upstream-Status: inappropriate [OE specific]
+Signed-off-by: Olof Johansson <olofjn@axis.com>
+---
+ lib/rpmscript.c | 3 +--
+ 1 file changed, 1 insertion(+), 2 deletions(-)
+
+diff --git a/lib/rpmscript.c b/lib/rpmscript.c
+index 98d3f420d..6bf23c18c 100644
+--- a/lib/rpmscript.c
++++ b/lib/rpmscript.c
+@@ -430,8 +430,7 @@ exit:
+ 	Fclose(out);	/* XXX dup'd STDOUT_FILENO */
+ 
+     if (fn) {
+-	if (!rpmIsDebug())
+-	    unlink(fn);
++	unlink(fn);
+ 	free(fn);
+     }
+     free(mline);
+-- 
+2.11.0
+
diff --git a/meta/recipes-devtools/rpm/rpm_4.14.1.bb b/meta/recipes-devtools/rpm/rpm_4.14.1.bb
index e5e87d39039..63a00ea1c0b 100644
--- a/meta/recipes-devtools/rpm/rpm_4.14.1.bb
+++ b/meta/recipes-devtools/rpm/rpm_4.14.1.bb
@@ -45,6 +45,12 @@ SRC_URI = "git://github.com/rpm-software-management/rpm;branch=rpm-4.14.x \
            file://0003-rpmSetCloseOnExec-use-getrlimit.patch \
            "
 
+# When we use RPM internally in OpenEmbedded, we enable debug output. This
+# unfortunately makes rpm not remove generated temporary files, which causes
+# incremental leakage for every build. Change behavior in rpm-native to always
+# remove the temporary files.
+SRC_URI_append_class-native = " file://0001-Always-remove-written-tmp-scriptlets-even-with-debug.patch"
+
 PE = "1"
 SRCREV = "bfee1410af51c1cc9724791fb8d985260a62102b"
 
-- 
2.11.0

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Alexander Kanavin]

2018-06-25 11:27 GMT+02:00 Olof Johansson <olof.johansson@axis.com>:
> RPM writes each package scriptlet (post-/preinstall) to /var/tmp/rpm-tmp.XXXXXX
>  --- a lot of files get created. When debugging is enabled, these temporary
> scriptlet files aren't cleaned up at all and after a while this results in the
> filesystem resources are eaten up (like running out of available inodes).
>
> This is a quick fix to avoid this. It does degrade functionality for
> those working with debugging pre-/postintall scripts.

It's better to clean them up in meta/lib/oe/rootfs.py, instead of
forever carrying a custom patch to rpm.

Alex

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Mark Hatle]

On 6/25/18 8:25 AM, Alexander Kanavin wrote:
> 2018-06-25 11:27 GMT+02:00 Olof Johansson <olof.johansson@axis.com>:
>> RPM writes each package scriptlet (post-/preinstall) to /var/tmp/rpm-tmp.XXXXXX
>>  --- a lot of files get created. When debugging is enabled, these temporary
>> scriptlet files aren't cleaned up at all and after a while this results in the
>> filesystem resources are eaten up (like running out of available inodes).
>>
>> This is a quick fix to avoid this. It does degrade functionality for
>> those working with debugging pre-/postintall scripts.
> 
> It's better to clean them up in meta/lib/oe/rootfs.py, instead of
> forever carrying a custom patch to rpm.

My question is "why is this a problem"?  (Why is debugging on for normal usage?)

If you want to enable debugging with RPM, leaving the files is the right answer.
 Otherwise, as mentioned in the post it's REALLY hard to debug scriptlet failures.

As for the rootfs.py cleaning them up, it's simply not possible -- assuming they
end up in a shared tmp dir, as you wouldn't have any idea who created them..
this build, or another, or a system process....  (Now if they only install into
the image's /var/tmp, thats a completely different case -- but I still question
why debug is enabled at all.)

--Mark

> Alex
> 

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Alexander Kanavin]

2018-06-25 18:47 GMT+02:00 Mark Hatle <mark.hatle@windriver.com>:
> My question is "why is this a problem"?  (Why is debugging on for normal usage?)
>
> If you want to enable debugging with RPM, leaving the files is the right answer.
>  Otherwise, as mentioned in the post it's REALLY hard to debug scriptlet failures.

Not at all. I've done quite a bit of scriptlet failure debugging (in
preparation for turning their failures into actual bitbake failures),
and haven't had to look at stuff in /var/tmp once. I'm fine with
deleting them.

When debugging is enabled, rpm writes each line of the scriptles as it
is being executed into the log, so you don't generally need to look at
the whole scriptlet. And even if you do, it's written as plaintext
into the beginning of the rpm file, so just look at it with 'less'.

> As for the rootfs.py cleaning them up, it's simply not possible -- assuming they
> end up in a shared tmp dir, as you wouldn't have any idea who created them..
> this build, or another, or a system process....  (Now if they only install into
> the image's /var/tmp, thats a completely different case -- but I still question
> why debug is enabled at all.)

Then we should write them to the rootfs /var/tmp and not the one on
the host. Or even ${T} where the rest of rpm/dnf logs go. Then they
don't need to be cleaned at all.

> This suggests some (probably[*]) larger, upstreamable work done
> in rpm itself, to be able to configure that behavior. I think
> this is better than nothing, for now. Perhaps the bug report
> shouldn't be closed, and this be merged as a workaround? (It
> causes real issues, at least for images with lots of
> pre/postinstalls.)

Sorry but no. I do not want 'workarounds' as merging them removes all
incentive for you to develop a proper fix.

> Does anybody see value in what --rpmverbosity=debug brings, as
> opposed to the default level of info? Would dropping that from
> lib/oe/package_manager.py's RpmPM._invoke_dnf be a better
> solution, or workaround?

We used to have that configurable, but the default level of
information is rather useless when things go wrong, and *especially*
when scriptlets fail. So recently that has changed to always enable
debug.

Alex

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Alexander Kanavin]

The temporary files are written here (macros.in file):

%_tmppath               %{_var}/tmp

What I don't understand is why they end up in the host /var and not
the rootfs one. The rpm database location is deduced from to _var as
well, and it does get created in the correct location. Further
investigation needed...

Alex

2018-06-25 21:20 GMT+02:00 Alexander Kanavin <alex.kanavin@gmail.com>:
> 2018-06-25 18:47 GMT+02:00 Mark Hatle <mark.hatle@windriver.com>:
>> My question is "why is this a problem"?  (Why is debugging on for normal usage?)
>>
>> If you want to enable debugging with RPM, leaving the files is the right answer.
>>  Otherwise, as mentioned in the post it's REALLY hard to debug scriptlet failures.
>
> Not at all. I've done quite a bit of scriptlet failure debugging (in
> preparation for turning their failures into actual bitbake failures),
> and haven't had to look at stuff in /var/tmp once. I'm fine with
> deleting them.
>
> When debugging is enabled, rpm writes each line of the scriptles as it
> is being executed into the log, so you don't generally need to look at
> the whole scriptlet. And even if you do, it's written as plaintext
> into the beginning of the rpm file, so just look at it with 'less'.
>
>> As for the rootfs.py cleaning them up, it's simply not possible -- assuming they
>> end up in a shared tmp dir, as you wouldn't have any idea who created them..
>> this build, or another, or a system process....  (Now if they only install into
>> the image's /var/tmp, thats a completely different case -- but I still question
>> why debug is enabled at all.)
>
> Then we should write them to the rootfs /var/tmp and not the one on
> the host. Or even ${T} where the rest of rpm/dnf logs go. Then they
> don't need to be cleaned at all.
>
>> This suggests some (probably[*]) larger, upstreamable work done
>> in rpm itself, to be able to configure that behavior. I think
>> this is better than nothing, for now. Perhaps the bug report
>> shouldn't be closed, and this be merged as a workaround? (It
>> causes real issues, at least for images with lots of
>> pre/postinstalls.)
>
> Sorry but no. I do not want 'workarounds' as merging them removes all
> incentive for you to develop a proper fix.
>
>> Does anybody see value in what --rpmverbosity=debug brings, as
>> opposed to the default level of info? Would dropping that from
>> lib/oe/package_manager.py's RpmPM._invoke_dnf be a better
>> solution, or workaround?
>
> We used to have that configurable, but the default level of
> information is rather useless when things go wrong, and *especially*
> when scriptlets fail. So recently that has changed to always enable
> debug.
>
> Alex

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Alexander Kanavin]

I believe the culprit is likely this patch:
http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/meta/recipes-devtools/rpm/files/0001-When-cross-installing-execute-package-scriptlets-wit.patch

As scriptlets are executed outside of rpm's chroot, they are being
written into system's /var/tmp, not rootfs's. This patch should
probably be amended to prepend the rootfs path when scriptlets are
written out, and then we should be sorted.

Alex

2018-06-25 21:43 GMT+02:00 Alexander Kanavin <alex.kanavin@gmail.com>:
> The temporary files are written here (macros.in file):
>
> %_tmppath               %{_var}/tmp
>
> What I don't understand is why they end up in the host /var and not
> the rootfs one. The rpm database location is deduced from to _var as
> well, and it does get created in the correct location. Further
> investigation needed...
>
> Alex
>
> 2018-06-25 21:20 GMT+02:00 Alexander Kanavin <alex.kanavin@gmail.com>:
>> 2018-06-25 18:47 GMT+02:00 Mark Hatle <mark.hatle@windriver.com>:
>>> My question is "why is this a problem"?  (Why is debugging on for normal usage?)
>>>
>>> If you want to enable debugging with RPM, leaving the files is the right answer.
>>>  Otherwise, as mentioned in the post it's REALLY hard to debug scriptlet failures.
>>
>> Not at all. I've done quite a bit of scriptlet failure debugging (in
>> preparation for turning their failures into actual bitbake failures),
>> and haven't had to look at stuff in /var/tmp once. I'm fine with
>> deleting them.
>>
>> When debugging is enabled, rpm writes each line of the scriptles as it
>> is being executed into the log, so you don't generally need to look at
>> the whole scriptlet. And even if you do, it's written as plaintext
>> into the beginning of the rpm file, so just look at it with 'less'.
>>
>>> As for the rootfs.py cleaning them up, it's simply not possible -- assuming they
>>> end up in a shared tmp dir, as you wouldn't have any idea who created them..
>>> this build, or another, or a system process....  (Now if they only install into
>>> the image's /var/tmp, thats a completely different case -- but I still question
>>> why debug is enabled at all.)
>>
>> Then we should write them to the rootfs /var/tmp and not the one on
>> the host. Or even ${T} where the rest of rpm/dnf logs go. Then they
>> don't need to be cleaned at all.
>>
>>> This suggests some (probably[*]) larger, upstreamable work done
>>> in rpm itself, to be able to configure that behavior. I think
>>> this is better than nothing, for now. Perhaps the bug report
>>> shouldn't be closed, and this be merged as a workaround? (It
>>> causes real issues, at least for images with lots of
>>> pre/postinstalls.)
>>
>> Sorry but no. I do not want 'workarounds' as merging them removes all
>> incentive for you to develop a proper fix.
>>
>>> Does anybody see value in what --rpmverbosity=debug brings, as
>>> opposed to the default level of info? Would dropping that from
>>> lib/oe/package_manager.py's RpmPM._invoke_dnf be a better
>>> solution, or workaround?
>>
>> We used to have that configurable, but the default level of
>> information is rather useless when things go wrong, and *especially*
>> when scriptlets fail. So recently that has changed to always enable
>> debug.
>>
>> Alex

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Olof Johansson]

On 18-06-26 12:33 +0200, Alexander Kanavin wrote:
> I believe the culprit is likely this patch:
> http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/meta/recipes-devtools/rpm/files/0001-When-cross-installing-execute-package-scriptlets-wit.patch
> 
> As scriptlets are executed outside of rpm's chroot, they are being
> written into system's /var/tmp, not rootfs's. This patch should
> probably be amended to prepend the rootfs path when scriptlets are
> written out, and then we should be sorted.

Is that not a more invasive change than just removing the tmp
files unconditionally (especially in terms of maintainability)?
I'm not even sure how exactly to do that, but I'm obviously
missing something. For instance, is there a way to manipulate the
global rpmMacroContext temporarily?

But your theory seems to be right, no /var/tmp/rpm-tmp.* files
are created if i drop the patch (but some scripts fail, as
expected).

-- 
olofjn

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Alexander Kanavin]

2018-06-30 1:53 GMT+02:00 Olof Johansson <olof.johansson@axis.com>:
>> As scriptlets are executed outside of rpm's chroot, they are being
>> written into system's /var/tmp, not rootfs's. This patch should
>> probably be amended to prepend the rootfs path when scriptlets are
>> written out, and then we should be sorted.
>
> Is that not a more invasive change than just removing the tmp
> files unconditionally (especially in terms of maintainability)?
> I'm not even sure how exactly to do that, but I'm obviously
> missing something. For instance, is there a way to manipulate the
> global rpmMacroContext temporarily?

Removing the tmp files unconditionally is a *workaround* for the fact
that they are written into an incorrect location, due to a different
patch removing the use of chroots (so that scriptlets can use native
tools in a cross-install situation), but not amending temporary
scriptlet file paths otherwise. The correct location would be the
/var/tmp directory inside the rootfs.

The files are created here:

lib/rpmscript.c:    FD_t fd = rpmMkTempFile("/", &fn);

Ideally, "/" should've been replaced with rpmtsRootDir(ts), but ts is
not accessible from the function, or from its parent, or parent
parent. So yes, I concede this would be too invasive to pass that down
the call stack. So if you can amend the above patch of mine with your
change, commit message, and SOB, that should be best. These changes
really belong together, as a custom fix for the oe build environment.

Alex

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Olof Johansson]

On 18-06-30 08:37 +0200, Alexander Kanavin wrote:
> Ideally, "/" should've been replaced with rpmtsRootDir(ts), but ts is
> not accessible from the function, or from its parent, or parent
> parent. So yes, I concede this would be too invasive to pass that down
> the call stack. So if you can amend the above patch of mine with your
> change, commit message, and SOB, that should be best. These changes
> really belong together, as a custom fix for the oe build environment.

Thanks for the feedback, I just sent a v2 of the patch. Hope it
is in line with what you asked for.

-- 
olofjn

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Alexander Kanavin]

Thanks, looks good now!

Alex

2018-07-03 14:01 GMT+02:00 Olof Johansson <olof.johansson@axis.com>:
> On 18-06-30 08:37 +0200, Alexander Kanavin wrote:
>> Ideally, "/" should've been replaced with rpmtsRootDir(ts), but ts is
>> not accessible from the function, or from its parent, or parent
>> parent. So yes, I concede this would be too invasive to pass that down
>> the call stack. So if you can amend the above patch of mine with your
>> change, commit message, and SOB, that should be best. These changes
>> really belong together, as a custom fix for the oe build environment.
>
> Thanks for the feedback, I just sent a v2 of the patch. Hope it
> is in line with what you asked for.
>
> --
> olofjn

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Peter Kjellerstedt]

Once this hits master, it should also be cherry-picked to Rocko and Sumo. 
Should be straightforward as the patch hasn't changed since it was 
introduced.

//Peter

> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of
> Alexander Kanavin
> Sent: den 3 juli 2018 14:33
> To: OE-core <openembedded-core@lists.openembedded.org>; Olof Johansson
> <Olof.Johansson@axis.com>
> Subject: Re: [OE-core] [PATCH] rpm: Avoid leaking temporary scriplet
> files
> 
> Thanks, looks good now!
> 
> Alex
> 
> 2018-07-03 14:01 GMT+02:00 Olof Johansson <olof.johansson@axis.com>:
> > On 18-06-30 08:37 +0200, Alexander Kanavin wrote:
> >> Ideally, "/" should've been replaced with rpmtsRootDir(ts), but ts
> is
> >> not accessible from the function, or from its parent, or parent
> >> parent. So yes, I concede this would be too invasive to pass that
> down
> >> the call stack. So if you can amend the above patch of mine with
> your
> >> change, commit message, and SOB, that should be best. These changes
> >> really belong together, as a custom fix for the oe build
> environment.
> >
> > Thanks for the feedback, I just sent a v2 of the patch. Hope it
> > is in line with what you asked for.
> >
> > --
> > olofjn
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: [PATCH] rpm: Avoid leaking temporary scriplet files

[Olof Johansson]

On 18-06-25 15:25 +0200, Alexander Kanavin wrote:
> It's better to clean them up in meta/lib/oe/rootfs.py, instead of
> forever carrying a custom patch to rpm.

I have the same hesitations, but we don't know the filenames to
clean up. We can't remove every /var/tmp/rpm-tmp.*, since they
can be created by unrelated rpm executions.

This suggests some (probably[*]) larger, upstreamable work done
in rpm itself, to be able to configure that behavior. I think
this is better than nothing, for now. Perhaps the bug report
shouldn't be closed, and this be merged as a workaround? (It
causes real issues, at least for images with lots of
pre/postinstalls.)

Does anybody see value in what --rpmverbosity=debug brings, as
opposed to the default level of info? Would dropping that from
lib/oe/package_manager.py's RpmPM._invoke_dnf be a better
solution, or workaround?

(Or maybe this is a bug in dnf? At least, this side effect is not
clearly documented, from what I can tell :))

-- 
olofjn

*) i'm not a c developer, so at least for me. :(