Skip to content

Better healthcheck correctness #1056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeremiahsnapp merged 5 commits into from
Jan 25, 2022
Merged

Better healthcheck correctness #1056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6652b83
Better healthcheck correctness
lamont-granquist Jan 13, 2022
4408377
Convert filename filter list to pure ruby
lamont-granquist Jan 13, 2022
0b75e71
Fix misc breakages and extract solaris-specific method
lamont-granquist Jan 21, 2022
0b6984e
Fix and add specs
lamont-granquist Jan 22, 2022
41e598a
extract freebsd method and fix it
lamont-granquist Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 147 additions & 38 deletions lib/omnibus/health_check.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

# Copyright 2012-2018 Chef Software, Inc.
# Copyright:: Copyright (c) Chef Software Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -66,20 +66,25 @@ def initialize(project)
def run!
measure("Health check time") do
log.info(log_key) { "Running health on #{project.name}" }
bad_libs = case Ohai["platform"]
when "mac_os_x"
health_check_otool
when "aix"
health_check_aix
when "windows"
# TODO: objdump -p will provided a very limited check of
# explicit dependencies on windows. Most dependencies are
# implicit and hence not detected.
log.warn(log_key) { "Skipping dependency health checks on Windows." }
{}
else
health_check_ldd
end
bad_libs, good_libs =
case Ohai["platform"]
when "mac_os_x"
health_check_otool
when "aix"
health_check_aix
when "windows"
# TODO: objdump -p will provided a very limited check of
# explicit dependencies on windows. Most dependencies are
# implicit and hence not detected.
log.warn(log_key) { "Skipping dependency health checks on Windows." }
[{}, {}]
when "solaris2"
health_check_solaris
when "freebsd", "openbsd", "netbsd"
health_check_freebsd
else
health_check_linux
end

unresolved = []
unreliable = []
Expand Down Expand Up @@ -167,6 +172,10 @@ def run!
raise HealthCheckFailed
end

if good_libs.keys.length == 0 && !windows?
raise "Internal error: no good libraries were found"
end

conflict_map = {}

conflict_map = relocation_check if relocation_checkable?
Expand Down Expand Up @@ -280,19 +289,20 @@ def relocation_check
def health_check_otool
current_library = nil
bad_libs = {}
good_libs = {}

read_shared_libs("find #{project.install_dir}/ -type f | egrep '\.(dylib|bundle)$' | xargs otool -L") do |line|
read_shared_libs("find #{project.install_dir}/ -type f | egrep '\.(dylib|bundle)$'", "xargs otool -L") do |line|
case line
when /^(.+):$/
current_library = Regexp.last_match[1]
when /^\s+(.+) \(.+\)$/
linked = Regexp.last_match[1]
name = File.basename(linked)
bad_libs = check_for_bad_library(bad_libs, current_library, name, linked)
bad_libs, good_libs = check_for_bad_library(bad_libs, good_libs, current_library, name, linked)
end
end

bad_libs
[bad_libs, good_libs]
end

#
Expand All @@ -304,57 +314,125 @@ def health_check_otool
def health_check_aix
current_library = nil
bad_libs = {}
good_libs = {}

read_shared_libs("find #{project.install_dir}/ -type f | xargs file | grep \"RISC System\" | awk -F: '{print $1}' | xargs -n 1 ldd") do |line|
read_shared_libs("find #{project.install_dir}/ -type f | xargs file | grep \"XCOFF\" | awk -F: '{print $1}'", "xargs -n 1 ldd") do |line|
case line
when /^(.+) needs:$/
current_library = Regexp.last_match[1]
log.debug(log_key) { "Analyzing dependencies for #{current_library}" }
when /^\s+(.+)$/
name = Regexp.last_match[1]
linked = Regexp.last_match[1]
bad_libs = check_for_bad_library(bad_libs, current_library, name, linked)
( bad_libs, good_libs ) = check_for_bad_library(bad_libs, good_libs, current_library, name, linked)
when /File is not an executable XCOFF file/ # ignore non-executable files
else
log.warn(log_key) { "Line did not match for #{current_library}\n#{line}" }
end
end

bad_libs
[bad_libs, good_libs]
end

#
# Run healthchecks against ldd.
# Run healthchecks on Solaris.
#
# @return [Hash<String, Hash<String, Hash<String, Int>>>]
# the bad libraries (library_name -> dependency_name -> satisfied_lib_path -> count)
#
def health_check_ldd
regexp_ends = ".*(" + IGNORED_ENDINGS.map { |e| e.gsub(/\./, '\.') }.join("|") + ")$"
regexp_patterns = IGNORED_PATTERNS.map { |e| ".*" + e.gsub(%r{/}, '\/') + ".*" }.join("|")
regexp = regexp_ends + "|" + regexp_patterns
def health_check_solaris
current_library = nil
bad_libs = {}
good_libs = {}

read_shared_libs("find #{project.install_dir}/ -type f | xargs file | grep \"ELF\" | awk -F: '{print $1}' | sed -e 's/:$//'", "xargs -n 1 ldd") do |line|
case line
when /^(.+):$/
current_library = Regexp.last_match[1]
log.debug(log_key) { "Analyzing dependencies for #{current_library}" }
when /^\s+(.+) \=\>\s+(.+)( \(.+\))?$/
name = Regexp.last_match[1]
linked = Regexp.last_match[2]
( bad_libs, good_libs ) = check_for_bad_library(bad_libs, good_libs, current_library, name, linked)
when /^\s+(.+) \(.+\)$/
next
when /^\s+statically linked$/
next
when /^\s+not a dynamic executable$/ # ignore non-executable files
else
log.warn(log_key) do
"Line did not match for #{current_library}\n#{line}"
end
end
end

[bad_libs, good_libs]
end

#
# Run healthchecks on FreeBSD
#
# @return [Hash<String, Hash<String, Hash<String, Int>>>]
# the bad libraries (library_name -> dependency_name -> satisfied_lib_path -> count)
#
def health_check_freebsd
current_library = nil
bad_libs = {}
good_libs = {}

read_shared_libs("find #{project.install_dir}/ -type f -regextype posix-extended ! -regex '#{regexp}' | xargs ldd") do |line|
read_shared_libs("find #{project.install_dir}/ -type f | xargs file | grep \"ELF\" | awk -F: '{print $1}' | sed -e 's/:$//'", "xargs ldd") do |line|
case line
when /^(.+):$/
current_library = Regexp.last_match[1]
log.debug(log_key) { "Analyzing dependencies for #{current_library}" }
when /^\s+(.+) \=\>\s+(.+)( \(.+\))?$/
name = Regexp.last_match[1]
linked = Regexp.last_match[2]
bad_libs = check_for_bad_library(bad_libs, current_library, name, linked)
( bad_libs, good_libs ) = check_for_bad_library(bad_libs, good_libs, current_library, name, linked)
when /^\s+(.+) \(.+\)$/
next
when /^\s+statically linked$/
next
when /^\s+libjvm.so/
when /^\s+not a dynamic executable$/ # ignore non-executable files
else
log.warn(log_key) do
"Line did not match for #{current_library}\n#{line}"
end
end
end

[bad_libs, good_libs]
end

#
# Run healthchecks against ldd.
#
# @return [Hash<String, Hash<String, Hash<String, Int>>>]
# the bad libraries (library_name -> dependency_name -> satisfied_lib_path -> count)
#
def health_check_linux
current_library = nil
bad_libs = {}
good_libs = {}

read_shared_libs("find #{project.install_dir}/ -type f", "xargs ldd") do |line|
case line
when /^(.+):$/
current_library = Regexp.last_match[1]
log.debug(log_key) { "Analyzing dependencies for #{current_library}" }
when /^\s+(.+) \=\>\s+(.+)( \(.+\))?$/
name = Regexp.last_match[1]
linked = Regexp.last_match[2]
( bad_libs, good_libs ) = check_for_bad_library(bad_libs, good_libs, current_library, name, linked)
when /^\s+(.+) \(.+\)$/
next
when /^\s+libjava.so/
when /^\s+statically linked$/
next
when /^\s+libmawt.so/
when /^\s+libjvm.so/ # FIXME: should remove if it doesn't blow up server
next
when /^\s+libjava.so/ # FIXME: should remove if it doesn't blow up server
next
when /^\s+libmawt.so/ # FIXME: should remove if it doesn't blow up server
next
when /^\s+not a dynamic executable$/ # ignore non-executable files
else
Expand All @@ -364,7 +442,7 @@ def health_check_ldd
end
end

bad_libs
[bad_libs, good_libs]
end

private
Expand Down Expand Up @@ -399,10 +477,40 @@ def whitelist_files
# @yield [String]
# each line
#
def read_shared_libs(command)
cmd = shellout(command)
cmd.stdout.each_line do |line|
yield line
def read_shared_libs(find_command, ldd_command, &output_proc)
#
# construct the list of files to check
#

find_output = shellout!(find_command).stdout.lines

find_output.reject! { |file| IGNORED_ENDINGS.any? { |ending| file.end_with?("#{ending}\n") } }

find_output.reject! { |file| IGNORED_SUBSTRINGS.any? { |substr| file.include?(substr) } }

if find_output.empty?
# probably the find_command is busted, it should never be empty or why are you using omnibus?
raise "Internal Error: Health Check found no lines"
end

if find_output.any? { |file| file !~ Regexp.new(project.install_dir) }
# every file in the find output should be within the install_dir
raise "Internal Error: Health Check lines not matching the install_dir"
end

#
# feed the list of files to the "ldd" command
#

# this command will typically fail if the last file isn't a valid lib/binary which happens often
Copy link

@adfoster-r7 adfoster-r7 Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I may have spotted a regression; or I'm just hitting a non-deterministic issue more frequently with these code changes

The scenario is - I've just upgraded to a newer version of Omnibus, and it looks like the ldd command here can break halfway through with this current pattern. It took a while to track down things as the status code here is getting ignored, so the error was being swallowed.

Example, reading 3 files and the 2nd ELF file causes an error:

[9] pry(#<Omnibus::HealthCheck>)> puts ldd_output = shellout(ldd_command, input: "/opt/project/file1\n/opt/project/file2\n/opt/project/file3").stdout
/opt/project/file1:
	not a dynamic executable
/opt/project/file2:
=> nil

We get a partial result in the current implementation, but if you extract the status code you can see it has failed an exitstatus of 135:

[11] pry(#<Omnibus::HealthCheck>)> puts ldd_output = shellout(ldd_command, input: "/opt/project/file1\n/opt/project/file2\n/opt/project/file3").result
NoMethodError: undefined method `result' for <Mixlib::ShellOut#1230: command: 'xargs ldd' process_status: #<Process::Status: pid 46708 exit 123> stdout: '/opt/project/file1:
	not a dynamic executable
/opt/project/file2:' stderr: 'ldd: exited with unknown exit code (135)' child_pid: 46708 environment: {} timeout: 7200 user:  group:  working_dir:  >:Mixlib::ShellOut
from (pry):11:in `block in read_shared_libs'

I actually thought there was a bug in shellout(...).stdout since it looked like partial reads of stdout.

For my current setup, this new code path skips multiple healthchecks as a result of exiting earlier than expected

Copy link

@adfoster-r7 adfoster-r7 Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial pull request: #1137

ldd_output = shellout(ldd_command, input: find_output.join).stdout

#
# do the output process to determine if the files are good or bad
#

ldd_output.each_line do |line|
output_proc.call(line)
end
end

Expand All @@ -420,7 +528,7 @@ def read_shared_libs(command)
#
# @return the modified bad_library hash
#
def check_for_bad_library(bad_libs, current_library, name, linked)
def check_for_bad_library(bad_libs, good_libs, current_library, name, linked)
safe = nil

whitelist_libs = case Ohai["platform"]
Expand Down Expand Up @@ -463,10 +571,11 @@ def check_for_bad_library(bad_libs, current_library, name, linked)
bad_libs[current_library][name][linked] = 1
end
else
good_libs[current_library] = true
log.debug(log_key) { " -> PASSED: #{name} is either whitelisted or safely provided." }
end

bad_libs
[bad_libs, good_libs]
end
end
end
32 changes: 23 additions & 9 deletions lib/omnibus/whitelist.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

# Copyright 2012-2020, Chef Software Inc.
# Copyright:: Copyright (c) Chef Software Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -177,28 +177,34 @@
/libkvm\.so/,
/libprocstat\.so/,
/libmd\.so/,
/libdl\.so/,
].freeze

IGNORED_ENDINGS = %w{
.TXT
.[ch]
.[ch]pp
.[eh]rl
.app
.appup
.bat
.beam
.c
.cc
.cmake
.conf
.cpp
.css
.e*rb
.erb
.erl
.feature
.gemspec
.gif
.gitignore
.gitkeep
.h*h
.h
.h
.hh
.hpp
.hrl
.html
.jar
.java
.jpg
Expand All @@ -210,6 +216,7 @@
.lua
.md
.mkd
.mo
.npmignore
.out
.packlist
Expand All @@ -219,21 +226,28 @@
.png
.pod
.properties
.py[oc]*
.r*html
.py
.pyc
.pyo
.rake
.rb
.rbs
.rdoc
.rhtml
.ri
.rpm
.rst
.scss
.sh
.sql
.svg
.toml
.tt
.ttf
.txt
.xml
.yml
COPYING
Gemfile
LICENSE
Makefile
Expand All @@ -243,7 +257,7 @@
license
}.freeze

IGNORED_PATTERNS = %w{
IGNORED_SUBSTRINGS = %w{
/build_info/
/licenses/
/LICENSES/
Expand Down
Loading