landscape-client should sanitize environment and close file descriptors on startup

Bug #348681 reported by Andreas Hasenack on 2009-03-25
4
Affects Status Importance Assigned to Milestone
Landscape Client
High
Christopher Armstrong
Landscape Server
High
Christopher Armstrong
landscape-client (Ubuntu)
Undecided
Unassigned
Nominated for Intrepid by Andreas Hasenack
Jaunty
Undecided
Unassigned

Bug Description

We came across a situation in ls3 where the parent landscape-client environment had a DEBIAN_HAS_FRONTEND variable set. This came from a previous upgrade where landscape-client itself was upgraded and restarted, inheriting this variable from dpkg.

landscape-client should clean these variables on startup as they can mess up with subsequent package activities. In the ls3 case, dpkg got stuck. This was suggested:

<cjwatson> andreas: I think I would probably recommend iterating over the environment and unsetting anything that starts with DEBIAN_ or DEBCONF_
<cjwatson> if you want to be more selective, then DEBCONF_* plus DEBIAN_FRONTEND, DEBIAN_HAS_FRONTEND, DEBIAN_PRIORITY is a complete list

Oops, and don't forget LANDSCAPE_ATTACHMENTS. Although it gets overwritten if used, I didn't check what happens if there are no attachments, i.e., if landscape-client empties it.

Changed in landscape-client:
importance: Undecided → High
Changed in landscape:
importance: Undecided → High
milestone: none → mountainview-pre-8
description: updated
Christopher Armstrong (radix) wrote :

closing the FDs is proving to be a bit tricky, and now I'm wondering if it's really necessary. Surely shells don't allow their FDs (other than STDIO) to be inherited by the children? How exactly could the debconf FD get into the landscape-client process?

Christopher Armstrong (radix) wrote :

Ok, I've attached a branch which cleans up environment variables but not FDs.

tags: added: review
Duncan McGreggor (oubiwann) wrote :

[1] Nice docstrings -- thanks for adding those!

[2] The ticket mentions LANDSCAPE_ATTACHMENTS. But I don't see the env var for it getting cleaned up.

[3] Can you confirm (by experiment or second opinions) that closing the FDs is needed? It if is, I'd vote for a new branch/ticket...

As long as you're good on point [2], +1.

Jamu Kakar (jkakar) wrote :

I agree with Duncan's points, +1 with the same conditions.

tags: removed: review
Jamu Kakar (jkakar) wrote :
Download full text (20.4 KiB)

I discovered something odd. If I run the tests with 'trial -r glib2
landscape', they all run and pass. If I run ./test the test suite
hangs, as below [1]. I tried ./test in trunk and the first time I ran
it died because of a strange IOError [2], and the subsequent times it
worked.

[1]

jkakar@rex:~/lab/radix/landscape-client/clean-up-environment$ ./test
Running unittests...
[landscape/lib/tests/test_bootstrap.py]
............................
----------------------------------------------------------------------
Ran 28 tests in 0.505s

OK
(tests=28, failures=0, errors=0)

[landscape/lib/tests/test_dbus_util.py]
........................
----------------------------------------------------------------------
Ran 24 tests in 2.607s

OK
(tests=24, failures=0, errors=0)

[landscape/lib/tests/test_timestamp.py]
...
----------------------------------------------------------------------
Ran 3 tests in 0.010s

OK
(tests=3, failures=0, errors=0)

[landscape/lib/tests/test_fetch.py]
.............
----------------------------------------------------------------------
Ran 13 tests in 0.034s

OK
(tests=13, failures=0, errors=0)

[landscape/lib/tests/test_lock.py]
....
----------------------------------------------------------------------
Ran 4 tests in 0.513s

OK
(tests=4, failures=0, errors=0)

[landscape/lib/tests/test_bpickle.py]
...........
----------------------------------------------------------------------
Ran 11 tests in 0.001s

OK
(tests=11, failures=0, errors=0)

[landscape/lib/tests/test_disk.py]
......
----------------------------------------------------------------------
Ran 6 tests in 0.065s

OK
(tests=6, failures=0, errors=0)

[landscape/lib/tests/test_monitor.py]
...........................
----------------------------------------------------------------------
Ran 27 tests in 0.067s

OK
(tests=27, failures=0, errors=0)

[landscape/lib/tests/test_persist.py]
.....................................................................................................
----------------------------------------------------------------------
Ran 101 tests in 0.350s

OK
(tests=101, failures=0, errors=0)

[landscape/lib/tests/test_bpickle_dbus.py]
....
----------------------------------------------------------------------
Ran 4 tests in 0.000s

OK
(tests=4, failures=0, errors=0)

[landscape/lib/tests/test_sysstats.py]
................
----------------------------------------------------------------------
Ran 16 tests in 0.109s

OK
(tests=16, failures=0, errors=0)

[landscape/lib/tests/test_process.py]
.....
----------------------------------------------------------------------
Ran 5 tests in 0.003s

OK
(tests=5, failures=0, errors=0)

[landscape/lib/tests/test_scriptcontent.py]
...
----------------------------------------------------------------------
Ran 3 tests in 0.000s

OK
(tests=3, failures=0, errors=0)

[landscape/package/tests/test_store.py]
..............................................
----------------------------------------------------------------------
Ran 46 tests in 7.771s

OK
(tests=46, failures=0, errors=0)

[landscape/package/tests/test_reporter.py]
..............................
---------------------------------------------------------...

Christopher Armstrong (radix) wrote :

Okay, so if the file descriptor is created with something like:

$ exec 5<whatever.txt

then this file descriptor *will* be inherited by subprocesses.

radix@lantern:~$ echo "hi" > foo.txt
radix@lantern:~$ exec 6<foo.txt
radix@lantern:~$ python -c 'import os; print os.read(6, 1024)'
hi

however, other file descriptors, like those created with redirects, are not passed to unrelated child processes: for example, doing "cat > foo.txt & python" does not allow "python" to inherit the file descriptor for foo.txt.

So we need to figure out exactly how those file descriptors that colin is talking about are created.

Changed in landscape-client:
milestone: none → 1.0.29
Christopher Armstrong (radix) wrote :

I've opened bug #352458 to track the FD issue.

description: updated
Christopher Armstrong (radix) wrote :

Merged to trunk.

Changed in landscape-client:
assignee: nobody → radix
status: New → Fix Committed
Changed in landscape:
assignee: nobody → radix
status: New → Fix Committed
Gabriele Monti (psicus78) wrote :

Patch to fix this bug

Changed in landscape:
status: Fix Committed → Fix Released
Changed in landscape:
milestone: mountainview-pre-8 → mountainview
status: Fix Released → Fix Committed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package landscape-client - 1.0.29-0ubuntu0.9.04.0

---------------
landscape-client (1.0.29-0ubuntu0.9.04.0) jaunty; urgency=low

  * New upstream bugfix release (LP: #358744)
    - Add a timeout to HTTP operations to avoid hanging (LP: #349737)
    - Clean up environment variables on startup to avoid propagating
      variables that will corrupt package installation (LP: #348681)
    - Clean up FDs on startup for the same reason (LP: #352458)
    - Catch and handle certain errors from smart (such as invalid package
      data) to avoid "stuck" Landscape activities (LP: #268745)
    - Don't print warnings meant for developers to the console (LP: #336669)

 -- Christopher Armstrong <email address hidden> Thu, 09 Apr 2009 17:09:50 -0400

Changed in landscape-client (Ubuntu Jaunty):
status: New → Fix Released
Changed in landscape:
status: Fix Committed → Fix Released
Changed in landscape-client:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers