All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Seebach <peter.seebach@windriver.com>
To: "Peter A. Bigot" <pab@pabigot.com>
Cc: openembedded-core@lists.openembedded.org
Subject: Re: [PATCH] pseudo: fix memory leak and missed privilege drop
Date: Tue, 3 Sep 2013 10:41:37 -0500	[thread overview]
Message-ID: <20130903104137.16fd7f6c@e6410-2> (raw)
In-Reply-To: <1377477606-17591-1-git-send-email-pab@pabigot.com>

On Sun, 25 Aug 2013 19:40:06 -0500
"Peter A. Bigot" <pab@pabigot.com> wrote:

> qemu.bbclass adds PSEUDO_UNLOAD=1 in qemu_run_binary to avoid reference to
> pseudo functions that may not exist in the target environment.  This patch
> detects the addition of that variable within the environment to which the
> call applies, even if not present in the parent environment.
> 
> As a side effect it fixes a memory leak.

The memory leak was quasi-intentional -- I was aware of it, but I felt that
freeing memory right before an exec() was probably silly.

I'm currently waffling on a design question: It's obvious that, if execve()
has PSEUDO_UNLOAD=1 in envp, we want pseudo to be unloaded.

What if execve() doesn't have PSEUDO_UNLOAD=1, but some other thing has
happened which caused PSEUDO_UNLOAD to be 1 in the process that's calling
execve(), but which had not yet resulted in pseudo being unloaded? For
instance, I think there's at least one thing (memory is weak today, so I
can't remember at all where, but I think it might have been one of the Python
popen()-relatives) which lets you specify an environment, and if you do, that
is the *entire* environment -- it does not inherit.

My intuition is that if PSEUDO_UNLOAD is 1 in *either* of these, that should
win. But then we want a way to say "no, really, don't unload pseudo", which
means I might need to check for something like PSEUDO_UNLOAD=0 in envp.

So my proposed logic would be:

* if envp contains a value for PSEUDO_UNLOAD, act according to that value
* otherwise if pseudo_get_value("PSEUDO_UNLOAD") exists, use that
* otherwise act as though PSEUDO_UNLOAD is not set

(Where "is not set" implies "make sure pseudo's environment variables are
all present and expected to work.)

... But all of this is secondary. I think we should put this patch in-tree
for now so we can have the bug go away, and I can then spend a while
navel-gazing and deciding what should go into the official pseudo tree, which
is currently in a state of disrepair. Excellent catch!

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.


      parent reply	other threads:[~2013-09-03 16:35 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-26  0:40 [PATCH] pseudo: fix memory leak and missed privilege drop Peter A. Bigot
2013-08-26  0:47 ` Peter A. Bigot
2013-08-26  4:25 ` Saul Wold
2013-08-26 15:54 ` Mark Hatle
2013-08-26 16:16   ` Peter A. Bigot
2013-09-03 15:41 ` Peter Seebach [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130903104137.16fd7f6c@e6410-2 \
    --to=peter.seebach@windriver.com \
    --cc=openembedded-core@lists.openembedded.org \
    --cc=pab@pabigot.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.