Merge pull request #174 from mingnus/2021-04-28-coverity-fixes

Fix issues detected by Coverity

Makefile.in

@@ -20,15 +20,18 @@ V=@
 
 PROGRAMS=\
 	bin/pdata_tools
-
-ifeq ("@TESTING@", "yes")
-PROGRAMS += bin/pdata_tools_dev
+DEV_TOOLS=\
+	bin/pdata_tools_dev
 TESTLIBS=\
 	lib/libft.so
-endif
 
-.PHONY: all
-all: $(PROGRAMS) $(TESTLIBS)
+.PHONY: all dev-tools
+all: $(PROGRAMS)
+dev-tools: $(DEV_TOOLS)
+
+ifeq ("@TESTING@", "yes")
+all += $(TESTLIB)
+endif
 
 include contrib/Makefile
 
@@ -84,10 +87,7 @@ COMMON_SOURCE=\
 	persistent-data/space_map.cc \
 	persistent-data/transaction_manager.cc \
 	persistent-data/validators.cc \
-	thin-provisioning/cache_stream.cc \
-	thin-provisioning/chunk_stream.cc \
 	thin-provisioning/device_tree.cc \
-	thin-provisioning/fixed_chunk_stream.cc \
 	thin-provisioning/human_readable_format.cc \
 	thin-provisioning/mapping_tree.cc \
 	thin-provisioning/metadata.cc \
@@ -95,7 +95,6 @@ COMMON_SOURCE=\
 	thin-provisioning/metadata_counter.cc \
 	thin-provisioning/metadata_dumper.cc \
 	thin-provisioning/override_emitter.cc \
-	thin-provisioning/pool_stream.cc \
 	thin-provisioning/restore_emitter.cc \
 	thin-provisioning/rmap_visitor.cc \
 	thin-provisioning/superblock.cc \
@@ -137,8 +136,12 @@ DEVTOOLS_SOURCE=\
 	dbg-lib/sm_show_traits.cc \
 	era/devel_commands.cc \
 	era/era_debug.cc \
+	thin-provisioning/cache_stream.cc \
+	thin-provisioning/chunk_stream.cc \
 	thin-provisioning/damage_generator.cc \
 	thin-provisioning/devel_commands.cc \
+	thin-provisioning/fixed_chunk_stream.cc \
+	thin-provisioning/pool_stream.cc \
 	thin-provisioning/thin_debug.cc \
 	thin-provisioning/thin_generate_damage.cc \
 	thin-provisioning/thin_generate_mappings.cc \
@@ -185,7 +188,8 @@ CXXFLAGS+=@CXXOPTIMISE_FLAG@
 CXXFLAGS+=@CXXDEBUG_FLAG@
 CXXFLAGS+=@CXX_STRERROR_FLAG@
 CXXFLAGS+=@LFS_FLAGS@
-INCLUDES+=-I$(TOP_BUILDDIR) -I$(TOP_DIR) -I$(TOP_DIR)/thin-provisioning
+CPPFLAGS?=@CPPFLAGS@
+CPPFLAGS+=-I$(TOP_BUILDDIR) -I$(TOP_DIR)
 LIBS:=-laio -lexpat -lboost_iostreams -ldl
 DEV_LIBS:=-lncurses
 
@@ -211,30 +215,21 @@ INSTALL_DIR = $(INSTALL) -m 755 -d
 INSTALL_PROGRAM = $(INSTALL) -m 755
 INSTALL_DATA = $(INSTALL) -p -m 644
 
-ifeq ("@TESTING@", "yes")
-TEST_INCLUDES=\
-	-I$(TOP_DIR) \
-	-Igoogletest/googlemock/include \
-	-Igoogletest/googletest/include
-else
-TEST_INCLUDES=
-endif
-
 .SUFFIXES: .d .txt .8
 
 %.o: %.cc
 	@echo "    [CXX] $<"
 	@mkdir -p $(dir $@)
-	$(V) $(CXX) -c $(INCLUDES) $(CXXFLAGS) -o $@ $<
-	$(V) $(CXX) -MM -MT $(subst .cc,.o,$<) $(INCLUDES) $(TEST_INCLUDES) $(CXXFLAGS) $< > $*.$$$$; \
+	$(V) $(CXX) -c $(CPPFLAGS) $(CXXFLAGS) -o $@ $<
+	$(V) $(CXX) -MM -MT $(subst .cc,.o,$<) $(CPPFLAGS) $(CXXFLAGS) $< > $*.$$$$; \
 	sed 's,\([^ :]*\)\.o[ :]*,\1.o \1.gmo $* : Makefile ,g' < $*.$$$$ > $*.d; \
 	$(RM) $*.$$$$
 
 %.o: %.c
 	@echo "    [CC] $<"
 	@mkdir -p $(dir $@)
-	$(V) $(CC) -c $(INCLUDES) $(CFLAGS) -o $@ $<
-	$(V) $(CC) -MM -MT $(subst .cc,.o,$<) $(INCLUDES) $(TEST_INCLUDES) $(CFLAGS) $< > $*.$$$$; \
+	$(V) $(CC) -c $(CPPFLAGS) $(CFLAGS) -o $@ $<
+	$(V) $(CC) -MM -MT $(subst .cc,.o,$<) $(CPPFLAGS) $(CFLAGS) $< > $*.$$$$; \
 	sed 's,\([^ :]*\)\.o[ :]*,\1.o \1.gmo $* : Makefile ,g' < $*.$$$$ > $*.d; \
 	$(RM) $*.$$$$
 
@@ -275,7 +270,7 @@ clean:
 	find . -name \*.o -delete
 	find . -name \*.gmo -delete
 	find . -name \*.d -delete
-	$(RM) $(PROGRAMS) lib/*.a lib/*.so
+	$(RM) $(PROGRAMS) $(DEV_TOOLS) lib/*.a lib/*.so
 
 distclean: clean
 	$(RM) config.cache config.log config.status configure.h version.h Makefile unit-tests/Makefile
@@ -360,7 +355,8 @@ install-rust-tools: man8/thin_metadata_pack.8 man8/thin_metadata_unpack.8 rust-t
 	$(INSTALL_DATA) man8/thin_metadata_pack.8 $(MANPATH)/man8
 	$(INSTALL_DATA) man8/thin_metadata_unpack.8 $(MANPATH)/man8
 
-ifeq ("@TESTING@", "yes")
+#----------------------------------------------------------------
+
 include unit-tests/Makefile
 
 LIBFT_SOURCE=\
@@ -376,11 +372,11 @@ lib/libft.so: $(LIBFT_OBJECTS)
 
 .PHONEY: functional-test unit-test
 
-functional-test: $(PROGRAMS) $(TESTLIBS)
+functional-test: $(PROGRAMS) $(DEV_TOOLS) $(TESTLIBS)
 	cd functional-tests && ./run-tests run
 
 test: functional-test unit-test
-endif
+
+#----------------------------------------------------------------
 
 -include $(DEPEND_FILES)
-

base/application.cc

@@ -25,8 +25,14 @@ command::die(string const &msg)
 }
 
 ::uint64_t
-command::parse_uint64(string const &str, string const &desc)
+command::parse_uint64(char const *str, char const *desc)
 {
+	if (!str) {
+		ostringstream out;
+		out << "Couldn't parse " << desc << ": NULL";
+		die(out.str());
+	}
+
 	try {
 		// FIXME: check trailing garbage is handled
 		return lexical_cast<::uint64_t>(str);

base/application.h

@@ -19,7 +19,7 @@ namespace base {
 		virtual ~command() {}
 
 		void die(std::string const &msg);
-		uint64_t parse_uint64(std::string const &str, std::string const &desc);
+		uint64_t parse_uint64(char const *str, char const *desc);
 
 
 		virtual void usage(std::ostream &out) const = 0;

base/file_utils.cc

@@ -155,8 +155,10 @@ file_utils::zero_superblock(std::string const &path)
         	throw runtime_error("out of memory");
 
 	memset(buffer, 0, SUPERBLOCK_SIZE);
-	if (::write(fd.fd_, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE)
+	if (::write(fd.fd_, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE) {
+		free(buffer);
 		throw runtime_error("couldn't zero superblock");
+	}
 }
 
 //----------------------------------------------------------------

block-cache/io_engine.cc

@@ -174,9 +174,6 @@ aio_engine::wait_(timespec *ts)
 		cbs_.free(cb);
 		return optional<wait_result>(make_pair(false, context));
 	}
-
-	// shouldn't get here
-	return optional<wait_result>(make_pair(false, 0));
 }
 
 struct timespec

caching/hint_array.cc

@@ -56,9 +56,6 @@ namespace {
 		default:
 			throw runtime_error("invalid hint width");
 		}
-
-		// never get here
-		return std::shared_ptr<array_base>();
 	}
 
 	//--------------------------------
@@ -93,9 +90,6 @@ namespace {
 		default:
 			throw runtime_error("invalid hint width");
 		}
-
-		// never get here
-		return std::shared_ptr<array_base>();
 	}
 
 	//--------------------------------

contrib/Makefile.in

@@ -18,7 +18,7 @@ contrib/%.a: contrib/%.o
 	$(V)echo "    [AR] $@"
 	$(V)$(AR) rcs $@ $^
 
-contrib/%.so: contrib/%.a
+contrib/%.so: contrib/%.o
 	$(V)echo "    [LD] $@"
 	$(V)$(CC) -shared -Wl,-soname,$@ -o $@ $<
 

era/restore_emitter.cc

@@ -14,6 +14,7 @@ namespace {
 			: md_(md),
 			  in_superblock_(false),
 			  in_writeset_(false),
+			  era_(0),
 			  in_era_array_(false) {
 		}
 

thin-provisioning/cache_stream.cc

@@ -62,7 +62,7 @@ chunk const &
 cache_stream::get()
 {
 	chunk_wrapper *w = new chunk_wrapper(*this);
-	return w->c_;
+	return w->c_; // wrapper will get freed by the put method
 }
 
 void

thin-provisioning/metadata_dumper.cc

@@ -180,7 +180,7 @@ namespace {
 // This is about classifying and summarising btree nodes.  The use of a btree
 // node may not be obvious when inspecting it in isolation.  But more information
 // may be gleaned by examining child and sibling nodes.
-// 
+//
 // So the process is:
 // - scan every metadata block, summarising it's potential uses.
 // - repeatedly iterate those summaries until we can glean no more useful information.
@@ -474,7 +474,7 @@ namespace {
 		node_info get_internal_info(block_manager::read_ref &rr) {
 			node_info info;
 			info.b = rr.get_location();
-			 
+
 			// values refer to blocks, so we should have infos for them.
 			auto n = to_node<block_traits>(rr);
 			::uint64_t key_low = 0;
@@ -524,7 +524,7 @@ namespace {
 			node_info info;
 			info.b = rr.get_location();
 
-			auto vsize = to_cpu<uint32_t>(hdr.value_size); 
+			auto vsize = to_cpu<uint32_t>(hdr.value_size);
 			info.values = to_cpu<uint32_t>(hdr.nr_entries);
 
 			if (vsize == sizeof(device_details_traits::disk_type)) {
@@ -645,6 +645,10 @@ namespace {
 	public:
 		mapping_emit_visitor(emitter::ptr e)
 			: e_(e),
+			  origin_start_(0),
+			  dest_start_(0),
+			  time_(0),
+			  len_(0),
 			  in_range_(false) {
 		}
 

thin-provisioning/shared_library_emitter.cc

@@ -8,22 +8,109 @@ using namespace thin_provisioning;
 
 //----------------------------------------------------------------
 
+struct shared_object {
+public:
+	shared_object(const char *shared_lib) {
+		handle_ = dlopen(shared_lib, RTLD_LAZY);
+		if (!handle_)
+			throw runtime_error(dlerror());
+
+		dlerror();    // Clear any existing error
+	}
+
+	virtual ~shared_object() {
+		dlclose(handle_);
+	}
+
+	void *get_symbol(const char *symbol) {
+		void *sym = dlsym(handle_, symbol);
+
+		char *error = dlerror();
+		if (error)
+			throw runtime_error(error);
+
+		return sym;
+	}
+
+	void *handle_;
+};
+
+class shared_emitter : public emitter {
+public:
+	shared_emitter(const char *shared_lib, ostream &out): sobj_(shared_lib) {
+		emitter::ptr (*create_fn)(ostream &out);
+		create_fn = reinterpret_cast<emitter::ptr (*)(ostream &)>(
+				sobj_.get_symbol("create_emitter"));
+		inner_ = create_fn(out);
+	}
+
+	virtual ~shared_emitter() {
+	}
+
+	void begin_superblock(std::string const &uuid,
+			      uint64_t time,
+			      uint64_t trans_id,
+			      boost::optional<uint32_t> flags,
+			      boost::optional<uint32_t> version,
+			      uint32_t data_block_size,
+			      uint64_t nr_data_blocks,
+			      boost::optional<uint64_t> metadata_snap) {
+		inner_->begin_superblock(uuid,
+					 time,
+					 trans_id,
+					 flags,
+					 version,
+					 data_block_size,
+					 nr_data_blocks,
+					 metadata_snap);
+	}
+
+	void end_superblock() {
+		inner_->end_superblock();
+	}
+
+	void begin_device(uint32_t dev_id,
+			  uint64_t mapped_blocks,
+			  uint64_t trans_id,
+			  uint64_t creation_time,
+			  uint64_t snap_time) {
+		inner_->begin_device(dev_id, mapped_blocks, trans_id, creation_time, snap_time);
+	}
+
+	void end_device() {
+		inner_->end_device();
+	}
+
+	void begin_named_mapping(std::string const &name) {
+		inner_->begin_named_mapping(name);
+	}
+
+	void end_named_mapping() {
+		inner_->end_named_mapping();
+	}
+
+	void identifier(std::string const &name) {
+		inner_->identifier(name);
+	}
+
+	void range_map(uint64_t origin_begin, uint64_t data_begin, uint32_t time, uint64_t len) {
+		inner_->range_map(origin_begin, data_begin, time, len);
+	}
+
+	void single_map(uint64_t origin_block, uint64_t data_block, uint32_t time) {
+		inner_->single_map(origin_block, data_block, time);
+	}
+
+	shared_object sobj_;
+	emitter::ptr inner_;
+};
+
+//----------------------------------------------------------------
+
 emitter::ptr
 thin_provisioning::create_custom_emitter(string const &shared_lib, ostream &out)
 {
-	emitter::ptr (*create_fn)(ostream &out);
-	void *handle = dlopen(shared_lib.c_str(), RTLD_LAZY);
-	if (!handle)
-		throw runtime_error(dlerror());
-
-	dlerror();    // Clear any existing error
-	create_fn = reinterpret_cast<emitter::ptr (*)(ostream &)>(dlsym(handle, "create_emitter"));
-
-	char *error = dlerror();
-	if (error)
-		throw runtime_error(error);
-
-	return create_fn(out);
+	return emitter::ptr(new shared_emitter(shared_lib.c_str(), out));
 }
 
 //----------------------------------------------------------------

thin-provisioning/thin_delta.cc

@@ -214,7 +214,9 @@ namespace {
 	class simple_emitter : public diff_emitter {
 	public:
 		simple_emitter(indented_stream &out)
-		: diff_emitter(out) {
+		: diff_emitter(out),
+		  vbegin_(0),
+		  vend_(0) {
 		}
 
 		void left_only(uint64_t vbegin, uint64_t dbegin, uint64_t len) {

thin-provisioning/thin_dump.cc

@@ -143,7 +143,6 @@ thin_dump_cmd::run(int argc, char **argv)
 	int c;
 	char const *output = NULL;
 	const char shortopts[] = "hm::o:f:rV";
-	char *end_ptr;
 	block_address metadata_snap = 0;
 	::uint64_t dev_id;
 	struct flags flags;
@@ -181,13 +180,7 @@ thin_dump_cmd::run(int argc, char **argv)
 			flags.use_metadata_snap = true;
 			if (optarg) {
 				// FIXME: deprecate this option
-				metadata_snap = strtoull(optarg, &end_ptr, 10);
-				if (end_ptr == optarg) {
-					cerr << "couldn't parse <metadata-snap>" << endl;
-					usage(cerr);
-					return 1;
-				}
-
+				metadata_snap = parse_uint64(optarg, "metadata-snap");
 				flags.snap_location = metadata_snap;
 			}
 			break;
@@ -197,12 +190,7 @@ thin_dump_cmd::run(int argc, char **argv)
 			break;
 
 		case 1:
-			dev_id = strtoull(optarg, &end_ptr, 10);
-			if (end_ptr == optarg) {
-				cerr << "couldn't parse <dev-id>\n";
-				usage(cerr);
-				return 1;
-			}
+			dev_id = parse_uint64(optarg, "dev-id");
 			flags.opts.select_dev(dev_id);
 			break;
 

thin-provisioning/thin_metadata_size.cc

@@ -192,9 +192,13 @@ static void printf_aligned(struct global *g, char const *a, char const *b, char
 {
 	char buf[80];
 
-	strcpy(buf, b);
-	if (units)
-		strcat(buf, mandatory ? "{" :"["), strcat(buf, g->unit.chars), strcat(buf, mandatory ? "}" : "]");
+	if (units) {
+		char left_bracket = mandatory ? '{' : '[';
+		char right_bracket = mandatory ? '}' : ']';
+		snprintf(buf, 80, "%s%c%s%c", b, left_bracket, g->unit.chars, right_bracket);
+	} else {
+		snprintf(buf, 80, "%s", b);
+	}
 
 	printf("\t%-4s%-44s%s\n", a, buf, c);
 }

thin-provisioning/thin_show_duplicates.cc

@@ -56,7 +56,7 @@ using namespace thin_provisioning;
 
 namespace {
 	bool factor_of(block_address f, block_address n) {
-		return (n % f) == 0;
+		return f && (n % f) == 0;
 	}
 
 	uint64_t parse_int(string const &str, string const &desc) {
@@ -132,11 +132,15 @@ namespace {
 	class duplicate_detector {
 	public:
 		void scan_with_variable_sized_chunks(chunk_stream &stream) {
+			if (!stream.size())
+				return;
 			variable_chunk_stream vstream(stream, 4096);
 			scan(vstream);
 		}
 
 		void scan_with_fixed_sized_chunks(chunk_stream &stream, block_address chunk_size) {
+			if (!stream.size())
+				return;
 			fixed_chunk_stream fstream(stream, chunk_size);
 			scan(fstream);
 		}
@@ -222,7 +226,7 @@ namespace {
 		if (fs.content_based_chunks)
 			detector.scan_with_variable_sized_chunks(pstream);
 		else {
-			if (*fs.block_size) {
+			if (!!fs.block_size) {
 				if (factor_of(*fs.block_size, block_size))
 					block_size = *fs.block_size;
 				else

thin-provisioning/thin_show_metadata.cc

@@ -206,7 +206,8 @@ namespace {
 
 		void run() {
 			auto line_length = 80;
-			for (block_address b = 0; b < 2000; b++) {
+			block_address nr_blocks = std::min<block_address>(2000, bm_.get_nr_blocks());
+			for (block_address b = 0; b < nr_blocks; b++) {
 				block_manager::read_ref rr = bm_.read_lock(b);
 
 				if (!(b % line_length)) {

unit-tests/Makefile.in

@@ -81,9 +81,9 @@ TEST_OBJECTS=$(subst .cc,.gmo,$(TEST_SOURCE))
 %.gmo: %.cc
 	@echo "    [CXX] $<"
 	@mkdir -p $(dir $@)
-	$(V) $(CXX) -c $(TEST_INCLUDES) $(CPPFLAGS) $(GMOCK_INCLUDES) $(CXXFLAGS) $(GMOCK_FLAGS) -o $@ $<
+	$(V) $(CXX) -c $(CPPFLAGS) $(GMOCK_INCLUDES) $(CXXFLAGS) $(GMOCK_FLAGS) -o $@ $<
 	@echo "    [DEP] $<"
-	$(V) $(CXX) -MM -MT $(subst .cc,.o,$<) $(TEST_INCLUDES) $(CPPFLAGS) $(GMOCK_INCLUDES) $(CXXFLAGS) $(GMOCK_FLAGS) $< > $*.$$$$; \
+	$(V) $(CXX) -MM -MT $(subst .cc,.o,$<) $(CPPFLAGS) $(GMOCK_INCLUDES) $(CXXFLAGS) $(GMOCK_FLAGS) $< > $*.$$$$; \
 	sed 's,\([^ :]*\)\.o[ :]*,\1.o \1.gmo $* : Makefile ,g' < $*.$$$$ > $*.d; \
 	$(RM) $*.$$$$