[PATCH] QemuRunner: avoid tainting os.environ

[PATCH] QemuRunner: avoid tainting os.environ

[Patrick Ohly]

That a utility function permanently changes the process environment is
bad style and leads to subtle, hard to debug problems.

For example, we had one oe-selftest which used runqemu() with an
override for DEPLOY_DIR_IMAGE. Another test then just called runCmd()
and ended up passing the wrong DEPLOY_DIR_IMAGE set earlier in
os.environ.

The approach used here is to extend the launch command such that 'env'
sets the environment variables. The specific values here should be
safe to use this way, even without extra quoting. This approach is
simple and has the advantage that the existing log.info('launchcmd=')
output includes the environment variables.

A more complex approach would be to pass a modified environment hash
to the subprocess module.

[YOCTO #11443]

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/lib/oeqa/utils/qemurunner.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
index ba44b96..c543797 100644
--- a/meta/lib/oeqa/utils/qemurunner.py
+++ b/meta/lib/oeqa/utils/qemurunner.py
@@ -98,11 +98,12 @@ class QemuRunner:
                 raise SystemExit
 
     def start(self, qemuparams = None, get_ip = True, extra_bootparams = None, runqemuparams='', launch_cmd=None, discard_writes=True):
+        env = []
         if self.display:
-            os.environ["DISPLAY"] = self.display
+            env.append("DISPLAY=" + self.display)
             # Set this flag so that Qemu doesn't do any grabs as SDL grabs
             # interact badly with screensavers.
-            os.environ["QEMU_DONT_GRAB"] = "1"
+            env.append("QEMU_DONT_GRAB=1")
         if not os.path.exists(self.rootfs):
             logger.error("Invalid rootfs %s" % self.rootfs)
             return False
@@ -110,12 +111,12 @@ class QemuRunner:
             logger.error("Invalid TMPDIR path %s" % self.tmpdir)
             return False
         else:
-            os.environ["OE_TMPDIR"] = self.tmpdir
+            env.append("OE_TMPDIR=" + self.tmpdir)
         if not os.path.exists(self.deploy_dir_image):
             logger.error("Invalid DEPLOY_DIR_IMAGE path %s" % self.deploy_dir_image)
             return False
         else:
-            os.environ["DEPLOY_DIR_IMAGE"] = self.deploy_dir_image
+            env.append("DEPLOY_DIR_IMAGE=" + self.deploy_dir_image)
 
         if not launch_cmd:
             launch_cmd = 'runqemu %s %s ' % ('snapshot' if discard_writes else '', runqemuparams)
@@ -128,6 +129,9 @@ class QemuRunner:
                 launch_cmd += ' nographic'
             launch_cmd += ' %s %s' % (self.machine, self.rootfs)
 
+        if env:
+            launch_cmd = 'env %s %s' % (' '.join(env), launch_cmd)
+
         return self.launch(launch_cmd, qemuparams=qemuparams, get_ip=get_ip, extra_bootparams=extra_bootparams)
 
     def launch(self, launch_cmd, get_ip = True, qemuparams = None, extra_bootparams = None):

base-commit: ba2884f6ad3a4e746fc80cbd707f83fa8abd4210
-- 
git-series 0.9.1

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Richard Purdie]

On Wed, 2017-05-03 at 12:56 +0200, Patrick Ohly wrote:
> That a utility function permanently changes the process environment
> is
> bad style and leads to subtle, hard to debug problems.
> 
> For example, we had one oe-selftest which used runqemu() with an
> override for DEPLOY_DIR_IMAGE. Another test then just called runCmd()
> and ended up passing the wrong DEPLOY_DIR_IMAGE set earlier in
> os.environ.
> 
> The approach used here is to extend the launch command such that
> 'env'
> sets the environment variables. The specific values here should be
> safe to use this way, even without extra quoting. This approach is
> simple and has the advantage that the existing log.info('launchcmd=')
> output includes the environment variables.
> 
> A more complex approach would be to pass a modified environment hash
> to the subprocess module.
> 
> [YOCTO #11443]
> 
> Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
> ---
>  meta/lib/oeqa/utils/qemurunner.py | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/meta/lib/oeqa/utils/qemurunner.py
> b/meta/lib/oeqa/utils/qemurunner.py
> index ba44b96..c543797 100644
> --- a/meta/lib/oeqa/utils/qemurunner.py
> +++ b/meta/lib/oeqa/utils/qemurunner.py
> @@ -98,11 +98,12 @@ class QemuRunner:
>                  raise SystemExit
>  
>      def start(self, qemuparams = None, get_ip = True,
> extra_bootparams = None, runqemuparams='', launch_cmd=None,
> discard_writes=True):
> +        env = []
>          if self.display:
> -            os.environ["DISPLAY"] = self.display
> +            env.append("DISPLAY=" + self.display)
>              # Set this flag so that Qemu doesn't do any grabs as SDL
> grabs
>              # interact badly with screensavers.
> -            os.environ["QEMU_DONT_GRAB"] = "1"
> +            env.append("QEMU_DONT_GRAB=1")
>          if not os.path.exists(self.rootfs):
>              logger.error("Invalid rootfs %s" % self.rootfs)
>              return False
> @@ -110,12 +111,12 @@ class QemuRunner:
>              logger.error("Invalid TMPDIR path %s" % self.tmpdir)
>              return False
>          else:
> -            os.environ["OE_TMPDIR"] = self.tmpdir
> +            env.append("OE_TMPDIR=" + self.tmpdir)
>          if not os.path.exists(self.deploy_dir_image):
>              logger.error("Invalid DEPLOY_DIR_IMAGE path %s" %
> self.deploy_dir_image)
>              return False
>          else:
> -            os.environ["DEPLOY_DIR_IMAGE"] = self.deploy_dir_image
> +            env.append("DEPLOY_DIR_IMAGE=" + self.deploy_dir_image)
>  
>          if not launch_cmd:
>              launch_cmd = 'runqemu %s %s ' % ('snapshot' if
> discard_writes else '', runqemuparams)
> @@ -128,6 +129,9 @@ class QemuRunner:
>                  launch_cmd += ' nographic'
>              launch_cmd += ' %s %s' % (self.machine, self.rootfs)
>  
> +        if env:
> +            launch_cmd = 'env %s %s' % (' '.join(env), launch_cmd)
> +
>          return self.launch(launch_cmd, qemuparams=qemuparams,
> get_ip=get_ip, extra_bootparams=extra_bootparams)
>  
>      def launch(self, launch_cmd, get_ip = True, qemuparams = None,
> extra_bootparams = None):

Why not pass in an env to the subprocess call in launch()?

Cheers,

Richard

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Patrick Ohly]

On Wed, 2017-05-03 at 12:13 +0100, Richard Purdie wrote:
> Why not pass in an env to the subprocess call in launch()?

Then it wouldn't get logged. Not a particularly strong argument, though.
If that's preferred, I can change the implementation.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On 3 May 2017 at 12:53, Patrick Ohly <patrick.ohly@intel.com> wrote:

> Then it wouldn't get logged. Not a particularly strong argument, though.
> If that's preferred, I can change the implementation.
>

I prefer the clarity of copying os.environ and adding to it over running
env, especially as you have whitespace/quoting to deal with.  The variables
added to the environment could easily be logged separately, surely?

Ross

[-- Attachment #2: Type: text/html, Size: 837 bytes --]

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Patrick Ohly]

On Wed, 2017-05-03 at 13:00 +0100, Burton, Ross wrote:
> 
> On 3 May 2017 at 12:53, Patrick Ohly <patrick.ohly@intel.com> wrote:
>         Then it wouldn't get logged. Not a particularly strong
>         argument, though.
>         If that's preferred, I can change the implementation.
>         
> 
> 
> I prefer the clarity of copying os.environ and adding to it over
> running env, especially as you have whitespace/quoting to deal with.
> The variables added to the environment could easily be logged
> separately, surely?

They could be logged in start(), but at the moment the logging is in
launch(), and if we keep the semantic the same as in subprocess.Popen()
(i.e. caller provides entire environment), then that logging doesn't
know which env variables might be worth logging.

Anyway, the variables weren't logged before either, so I'll just keep
that and change to passing an env dict.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On 3 May 2017 at 13:17, Patrick Ohly <patrick.ohly@intel.com> wrote:

> They could be logged in start(), but at the moment the logging is in
> launch(), and if we keep the semantic the same as in subprocess.Popen()
> (i.e. caller provides entire environment), then that logging doesn't
> know which env variables might be worth logging.
>

If you really cared, you could log the difference between the passed in env
and os.environ... :)
Ross

[-- Attachment #2: Type: text/html, Size: 837 bytes --]

[PATCH] QemuRunner: avoid tainting os.environ

[Patrick Ohly]

That a utility function permanently changes the process environment is
bad style and leads to subtle, hard to debug problems.

For example, we had one oe-selftest which used runqemu() with an
override for DEPLOY_DIR_IMAGE. Another test then just called runCmd()
and ended up passing the wrong DEPLOY_DIR_IMAGE set earlier in
os.environ.

The approach used here is to pass the desired environment dict to the
launch() method as a new, optional parameter, which then gets passed
on to subproject.Popen(). The modified env variables do not get
logged, as before.

[YOCTO #11443]

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/lib/oeqa/utils/qemurunner.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
index ba44b96..b06ccc8 100644
--- a/meta/lib/oeqa/utils/qemurunner.py
+++ b/meta/lib/oeqa/utils/qemurunner.py
@@ -98,11 +98,12 @@ class QemuRunner:
                 raise SystemExit
 
     def start(self, qemuparams = None, get_ip = True, extra_bootparams = None, runqemuparams='', launch_cmd=None, discard_writes=True):
+        env = dict(os.environ.items())
         if self.display:
-            os.environ["DISPLAY"] = self.display
+            env["DISPLAY"] = self.display
             # Set this flag so that Qemu doesn't do any grabs as SDL grabs
             # interact badly with screensavers.
-            os.environ["QEMU_DONT_GRAB"] = "1"
+            env["QEMU_DONT_GRAB"] = "1"
         if not os.path.exists(self.rootfs):
             logger.error("Invalid rootfs %s" % self.rootfs)
             return False
@@ -110,12 +111,12 @@ class QemuRunner:
             logger.error("Invalid TMPDIR path %s" % self.tmpdir)
             return False
         else:
-            os.environ["OE_TMPDIR"] = self.tmpdir
+            env["OE_TMPDIR"] = self.tmpdir
         if not os.path.exists(self.deploy_dir_image):
             logger.error("Invalid DEPLOY_DIR_IMAGE path %s" % self.deploy_dir_image)
             return False
         else:
-            os.environ["DEPLOY_DIR_IMAGE"] = self.deploy_dir_image
+            env["DEPLOY_DIR_IMAGE"] = self.deploy_dir_image
 
         if not launch_cmd:
             launch_cmd = 'runqemu %s %s ' % ('snapshot' if discard_writes else '', runqemuparams)
@@ -128,9 +129,9 @@ class QemuRunner:
                 launch_cmd += ' nographic'
             launch_cmd += ' %s %s' % (self.machine, self.rootfs)
 
-        return self.launch(launch_cmd, qemuparams=qemuparams, get_ip=get_ip, extra_bootparams=extra_bootparams)
+        return self.launch(launch_cmd, qemuparams=qemuparams, get_ip=get_ip, extra_bootparams=extra_bootparams, env=env)
 
-    def launch(self, launch_cmd, get_ip = True, qemuparams = None, extra_bootparams = None):
+    def launch(self, launch_cmd, get_ip = True, qemuparams = None, extra_bootparams = None, env = None):
         try:
             threadsock, threadport = self.create_socket()
             self.server_socket, self.serverport = self.create_socket()
@@ -157,7 +158,7 @@ class QemuRunner:
         # blocking at the end of the runqemu script when using this within
         # oe-selftest (this makes stty error out immediately). There ought
         # to be a proper fix but this will suffice for now.
-        self.runqemu = subprocess.Popen(launch_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, stdin=subprocess.PIPE, preexec_fn=os.setpgrp)
+        self.runqemu = subprocess.Popen(launch_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, stdin=subprocess.PIPE, preexec_fn=os.setpgrp, env=env)
         output = self.runqemu.stdout
 
         #

base-commit: ba2884f6ad3a4e746fc80cbd707f83fa8abd4210
-- 
git-series 0.9.1

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Richard Purdie]

On Wed, 2017-05-03 at 14:17 +0200, Patrick Ohly wrote:
> On Wed, 2017-05-03 at 13:00 +0100, Burton, Ross wrote:
> > 
> > 
> > On 3 May 2017 at 12:53, Patrick Ohly <patrick.ohly@intel.com>
> > wrote:
> >         Then it wouldn't get logged. Not a particularly strong
> >         argument, though.
> >         If that's preferred, I can change the implementation.
> >         
> > 
> > 
> > I prefer the clarity of copying os.environ and adding to it over
> > running env, especially as you have whitespace/quoting to deal
> > with.
> > The variables added to the environment could easily be logged
> > separately, surely?
> They could be logged in start(), but at the moment the logging is in
> launch(), and if we keep the semantic the same as in
> subprocess.Popen()
> (i.e. caller provides entire environment), then that logging doesn't
> know which env variables might be worth logging.
> 
> Anyway, the variables weren't logged before either, so I'll just keep
> that and change to passing an env dict.

You could pass in the specific env delta you want to the function and
leave launch to infill anything which isn't set?

Cheers,

Richard

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Patrick Ohly]

On Wed, 2017-05-03 at 13:53 +0100, Richard Purdie wrote:
> > Anyway, the variables weren't logged before either, so I'll just
> keep
> > that and change to passing an env dict.
> 
> You could pass in the specific env delta you want to the function and
> leave launch to infill anything which isn't set?

That behavior would then differ from how subprocess works and make it
harder to remove variables from the os.environ. I don't find that
appealing.

Forget that I ever brought up logging of env variables, okay? ;-}

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] QemuRunner: avoid tainting os.environ

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 161 bytes --]

On 3 May 2017 at 13:40, Patrick Ohly <patrick.ohly@intel.com> wrote:

> +        env = dict(os.environ.items())
>

os.environ.copy() seems neater.

Ross

[-- Attachment #2: Type: text/html, Size: 574 bytes --]

[PATCH] QemuRunner: avoid tainting os.environ

[Patrick Ohly]

That a utility function permanently changes the process environment is
bad style and leads to subtle, hard to debug problems.

For example, we had one oe-selftest which used runqemu() with an
override for DEPLOY_DIR_IMAGE. Another test then just called runCmd()
and ended up passing the wrong DEPLOY_DIR_IMAGE set earlier in
os.environ.

The approach used here is to pass the desired environment dict to the
launch() method as a new, optional parameter, which then gets passed
on to subproject.Popen(). The modified env variables do not get
logged, as before.

[YOCTO #11443]

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/lib/oeqa/utils/qemurunner.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
index ba44b96..cd60ba7 100644
--- a/meta/lib/oeqa/utils/qemurunner.py
+++ b/meta/lib/oeqa/utils/qemurunner.py
@@ -98,11 +98,12 @@ class QemuRunner:
                 raise SystemExit
 
     def start(self, qemuparams = None, get_ip = True, extra_bootparams = None, runqemuparams='', launch_cmd=None, discard_writes=True):
+        env = os.environ.copy()
         if self.display:
-            os.environ["DISPLAY"] = self.display
+            env["DISPLAY"] = self.display
             # Set this flag so that Qemu doesn't do any grabs as SDL grabs
             # interact badly with screensavers.
-            os.environ["QEMU_DONT_GRAB"] = "1"
+            env["QEMU_DONT_GRAB"] = "1"
         if not os.path.exists(self.rootfs):
             logger.error("Invalid rootfs %s" % self.rootfs)
             return False
@@ -110,12 +111,12 @@ class QemuRunner:
             logger.error("Invalid TMPDIR path %s" % self.tmpdir)
             return False
         else:
-            os.environ["OE_TMPDIR"] = self.tmpdir
+            env["OE_TMPDIR"] = self.tmpdir
         if not os.path.exists(self.deploy_dir_image):
             logger.error("Invalid DEPLOY_DIR_IMAGE path %s" % self.deploy_dir_image)
             return False
         else:
-            os.environ["DEPLOY_DIR_IMAGE"] = self.deploy_dir_image
+            env["DEPLOY_DIR_IMAGE"] = self.deploy_dir_image
 
         if not launch_cmd:
             launch_cmd = 'runqemu %s %s ' % ('snapshot' if discard_writes else '', runqemuparams)
@@ -128,9 +129,9 @@ class QemuRunner:
                 launch_cmd += ' nographic'
             launch_cmd += ' %s %s' % (self.machine, self.rootfs)
 
-        return self.launch(launch_cmd, qemuparams=qemuparams, get_ip=get_ip, extra_bootparams=extra_bootparams)
+        return self.launch(launch_cmd, qemuparams=qemuparams, get_ip=get_ip, extra_bootparams=extra_bootparams, env=env)
 
-    def launch(self, launch_cmd, get_ip = True, qemuparams = None, extra_bootparams = None):
+    def launch(self, launch_cmd, get_ip = True, qemuparams = None, extra_bootparams = None, env = None):
         try:
             threadsock, threadport = self.create_socket()
             self.server_socket, self.serverport = self.create_socket()
@@ -157,7 +158,7 @@ class QemuRunner:
         # blocking at the end of the runqemu script when using this within
         # oe-selftest (this makes stty error out immediately). There ought
         # to be a proper fix but this will suffice for now.
-        self.runqemu = subprocess.Popen(launch_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, stdin=subprocess.PIPE, preexec_fn=os.setpgrp)
+        self.runqemu = subprocess.Popen(launch_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, stdin=subprocess.PIPE, preexec_fn=os.setpgrp, env=env)
         output = self.runqemu.stdout
 
         #

base-commit: ba2884f6ad3a4e746fc80cbd707f83fa8abd4210
-- 
git-series 0.9.1