The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Bug #10909

Shell out when reading from /proc

Added by Josh Cooper over 4 years ago. Updated about 3 years ago.

Status:RejectedStart date:11/16/2011
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-
Keywords: customer Affected Facter version:
Branch:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


Description

There are many places in facter where it using ruby’s IO.read or IO.readlines to read the contents of files in /proc. This is problematic, as it can cause ruby to block definitely. It is better to use:

Facter::Util::Resolution.exec("/bin/cat #{uptime_file} 2>/dev/null")

with an appropriate timeout. This issue was triggering https://support.puppetlabs.com/tickets/482 (though not the cause)


Related issues

Related to Facter - Bug #7138: Facter use exec + cat ( or exec = hostname ) when not needed Merged - Pending Release 04/18/2011
Related to Puppet - Bug #7141: puppetd runs fail in 'daemon' mode when stat'ing /proc files Closed 04/18/2011 07/05/2011
Related to Puppet - Bug #10418: Puppet agent hangs when listen is true and reading from /... Closed 11/01/2011
Related to Facter - Bug #4466: Files in /proc should not be read directly by Ruby Closed 08/04/2010

History

#1 Updated by Matt Robinson over 4 years ago

This sounds worth doing an audit of the codebase to see where we can do this to protect ourselves against this problem since it seems to be recurring and we have about a dozen tickets related to the issue, the earliest appears to be #4466. It seems like it was fixed in the Kernel, but recently Redhat released a kernel version that regressed. The support ticket Josh mentions in the ticket description isn’t readable to most, so here’s an excerpt:

Using this command:

$ puppet agent --verbose --no-daemonize --debug --trace --server sirrus --listen

I get expected pass/fail across different kernel versions:
pass 2.6.18-238.el5 
fail 2.6.18-274.7.1.el5 
pass 2.6.18-298.el5.bz751214.1

Note facter uses IO.readlines and File.read to read from /proc in many places:

lib/facter/util/ip.rb:161: bondinfo = IO.readlines("/proc/net/bonding/#{bonddev}") 
lib/facter/util/memory.rb:12: File.readlines("/proc/meminfo").each do |l| 
lib/facter/util/processor.rb:12: File.readlines(cpuinfo).each do |l| 
lib/facter/util/processor.rb:27: File.readlines(cpuinfo).each do |l| 
lib/facter/util/processor.rb:39: File.readlines(cpuinfo).each do |l| 
lib/facter/util/processor.rb:55: File.readlines(cpuinfo).each do |l|

Facter should be using cat instead, with an appropriate timeout:
Facter::Util::Resolution.exec("/bin/cat #{uptime_file} 2>/dev/null")

The one concern I’ve heard raised about switching from using IO.read to shelling out is that if it’s something that happens frequently, the overhead of forking to run the shell command could slow things way down. I’ll keep that in mind when looking through the code.

#2 Updated by Anonymous over 4 years ago

  • Status changed from Accepted to Rejected

Working around bugs in long obsolete versions of the Linux kernel (eg: 2.6.13) is a bad strategy, and will result in more fragile, harder to maintain code.

Working around these because a vendor reintroduced the bug briefly as part of a kernel update doesn’t make that any wiser.

#3 Updated by Josh Cooper over 4 years ago

Working around these because a vendor reintroduced the bug briefly as part of a kernel update doesn’t make that any wiser.

Just to be clear, implementing this change wouldn’t have prevented the hang from occurring. It would occur anytime Puppet performed a blocking read with it’s listener thread blocked in accept, so pretty much all the time.

While reviewing this issue, Jacob said that facter shouldn’t be calling IO.readlines, etc, generally speaking, and so this is why I filed it.

#4 Updated by Anonymous over 4 years ago

Josh Cooper wrote:

Working around these because a vendor reintroduced the bug briefly as part of a kernel update doesn’t make that any wiser.

Just to be clear, implementing this change wouldn’t have prevented the hang from occurring. It would occur anytime Puppet performed a blocking read with it’s listener thread blocked in accept, so pretty much all the time.

That seems a radically different bug to the one found in 2.7.13, and reintroduced in the RedHat kernel, which was that select didn’t work correctly for files on the proc virtual file system; the relationship with select and listen came about, as I understood it, because the Ruby C kernel would use select if, and only if, there were two active file handles – and that cropped up when listening on a network socket and reading files.

While reviewing this issue, Jacob said that facter shouldn’t be calling IO.readlines, etc, generally speaking, and so this is why I filed it.

That seems odd, and it would have been very useful to include the rationale for that in this ticket. After all, if we are making design decisions like “don’t use standard file I/O in Facter” then we need to tell folks what they must do instead, and work out how to enforce that standard. After all, if these methods shouldn’t be used, having our users do so and break things is highly undesirable…

#5 Updated by Charlie Sharpsteen about 3 years ago

  • Keywords set to customer

Also available in: Atom PDF