[PATCH] Fix uninitialized variable with wrong type

Yuriy Skalko yuriy.skalko at gmail.com
Thu Sep 10 22:32:45 UTC 2020


> I do not see how returning an int valie which has different interpretations depending on the OS can be clean. I would propose to change the first element of cmd_ret with a boolean (if this is enough) or an enum (if we return more detailed information) and compute this value cleanly in each branch.
> 
> 
> Or spend this time implementing the following advice (no idea how easy it is):
> 
> 
>         // FIXME: replace all calls to RunCommand with ForkedCall
>         // (if the output is not needed) or the code in ISpell.cpp
>         // (if the output is needed).
> 
> 
> JMarc


I've done some refactoring according to the first advice. Yes, there
should be more separate handling of int/DWORD for different systems. I
think structure will be better here than pair. As I understand, boolean
is enough in this case.

Please check the attached patch.

As for the FIXME comment, I don't know where to get the ISpell.cpp file.


Yuriy


-------------- next part --------------
From 749b357e3d9db40a373017124ac097d4253e5e82 Mon Sep 17 00:00:00 2001
From: Yuriy Skalko <yuriy.skalko at gmail.com>
Date: Fri, 11 Sep 2020 00:22:55 +0300
Subject: [PATCH] Refactor runCommand

---
 src/Buffer.cpp            |  2 +-
 src/TextClass.cpp         |  2 +-
 src/mathed/MathExtern.cpp |  2 +-
 src/support/filetools.cpp | 30 +++++++++++++++++-------------
 src/support/filetools.h   |  2 +-
 src/support/os.cpp        |  4 ++--
 src/support/os_win32.cpp  |  4 ++--
 7 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 9de281daf3..82fd0899c4 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1357,7 +1357,7 @@ Buffer::ReadStatus Buffer::convertLyXFormat(FileName const & fn,
 	LYXERR(Debug::INFO, "Running '" << command_str << '\'');
 
 	cmd_ret const ret = runCommand(command_str);
-	if (ret.first != 0) {
+	if (!ret.valid) {
 		if (from_format < LYX_FORMAT) {
 			Alert::error(_("Conversion script failed"),
 				bformat(_("%1$s is from an older version"
diff --git a/src/TextClass.cpp b/src/TextClass.cpp
index eb92612d84..4cc5eba0ed 100644
--- a/src/TextClass.cpp
+++ b/src/TextClass.cpp
@@ -92,7 +92,7 @@ bool layout2layout(FileName const & filename, FileName const & tempfile,
 	LYXERR(Debug::TCLASS, "Running `" << command_str << '\'');
 
 	cmd_ret const ret = runCommand(command_str);
-	if (ret.first != 0) {
+	if (!ret.valid) {
 		if (format == LAYOUT_FORMAT)
 			LYXERR0("Conversion of layout with layout2layout.py has failed.");
 		return false;
diff --git a/src/mathed/MathExtern.cpp b/src/mathed/MathExtern.cpp
index 311b323895..18340e11c8 100644
--- a/src/mathed/MathExtern.cpp
+++ b/src/mathed/MathExtern.cpp
@@ -1026,7 +1026,7 @@ namespace {
 		       << "\ninput: '" << data << "'" << endl;
 		cmd_ret const ret = runCommand(command);
 		cas_tmpfile.removeFile();
-		return ret.second;
+		return ret.result;
 	}
 
 	size_t get_matching_brace(string const & str, size_t i)
diff --git a/src/support/filetools.cpp b/src/support/filetools.cpp
index d6b27856cb..ca26b0bde8 100644
--- a/src/support/filetools.cpp
+++ b/src/support/filetools.cpp
@@ -1095,38 +1095,42 @@ cmd_ret const runCommand(string const & cmd)
 	// (Claus Hentschel) Check if popen was successful ;-)
 	if (!inf) {
 		lyxerr << "RunCommand:: could not start child process" << endl;
-		return make_pair(-1, string());
+		return {false, string()};
 	}
 
-	string ret;
+	string result;
 	int c = fgetc(inf);
 	while (c != EOF) {
-		ret += static_cast<char>(c);
+		result += static_cast<char>(c);
 		c = fgetc(inf);
 	}
 
 #if defined (_WIN32)
 	WaitForSingleObject(process.hProcess, INFINITE);
 	DWORD pret;
-	if (!GetExitCodeProcess(process.hProcess, &pret))
-		pret = -1;
+	BOOL success = GetExitCodeProcess(process.hProcess, &pret);
+	bool valid = (pret == 0);
+	if (!success)
+		valid = false;
 	if (!infile.empty())
 		CloseHandle(startup.hStdInput);
 	CloseHandle(process.hProcess);
 	if (fclose(inf) != 0)
-		pret = -1;
+		valid = false;
 #elif defined (HAVE_PCLOSE)
 	int const pret = pclose(inf);
+	bool valid = (pret != -1);
 #elif defined (HAVE__PCLOSE)
 	int const pret = _pclose(inf);
+	bool valid = (pret != -1);
 #else
 #error No pclose() function.
 #endif
 
-	if (pret == -1)
+	if (!valid)
 		perror("RunCommand:: could not terminate child process");
 
-	return make_pair(pret, ret);
+	return {valid, result};
 }
 
 
@@ -1174,10 +1178,10 @@ FileName const findtexfile(string const & fil, string const & /*format*/,
 
 	cmd_ret const c = runCommand(kpsecmd);
 
-	LYXERR(Debug::LATEX, "kpse status = " << c.first << '\n'
-		 << "kpse result = `" << rtrim(c.second, "\n\r") << '\'');
-	if (c.first != -1)
-		return FileName(rtrim(to_utf8(from_filesystem8bit(c.second)), "\n\r"));
+	LYXERR(Debug::LATEX, "kpse status = " << c.valid << '\n'
+		 << "kpse result = `" << rtrim(c.result, "\n\r") << '\'');
+	if (c.valid)
+		return FileName(rtrim(to_utf8(from_filesystem8bit(c.result)), "\n\r"));
 	else
 		return FileName();
 }
@@ -1223,7 +1227,7 @@ bool prefs2prefs(FileName const & filename, FileName const & tempfile, bool lfun
 	LYXERR(Debug::FILES, "Running `" << command_str << '\'');
 
 	cmd_ret const ret = runCommand(command_str);
-	if (ret.first != 0) {
+	if (!ret.valid) {
 		LYXERR0("Could not run file conversion script prefs2prefs.py.");
 		return false;
 	}
diff --git a/src/support/filetools.h b/src/support/filetools.h
index b0f5f7f379..e8fa954851 100644
--- a/src/support/filetools.h
+++ b/src/support/filetools.h
@@ -328,7 +328,7 @@ bool prefs2prefs(FileName const & filename, FileName const & tempfile,
 /// Does file \p file need to be updated by configure.py?
 bool configFileNeedsUpdate(std::string const & file);
 
-typedef std::pair<int, std::string> cmd_ret;
+typedef struct {bool valid; std::string result;} cmd_ret;
 
 cmd_ret const runCommand(std::string const & cmd);
 
diff --git a/src/support/os.cpp b/src/support/os.cpp
index 616b9c6936..3a5c2e59a2 100644
--- a/src/support/os.cpp
+++ b/src/support/os.cpp
@@ -65,7 +65,7 @@ static string const python23_call(string const & binary, bool verbose = false)
 	smatch sm;
 	try {
 		static regex const python_reg("\\((\\d*), (\\d*)\\)");
-		if (out.first < 0 || !regex_match(out.second, sm, python_reg))
+		if (!out.valid || !regex_match(out.result, sm, python_reg))
 			return string();
 	} catch(regex_error const & /*e*/) {
 		LYXERR0("Regex error! This should not happen.");
@@ -78,7 +78,7 @@ static string const python23_call(string const & binary, bool verbose = false)
 		return string();
 
 	if (verbose)
-		lyxerr << "Found Python " << out.second << "\n";
+		lyxerr << "Found Python " << out.result << "\n";
 	// Add the -tt switch so that mixed tab/whitespace
 	// indentation is an error
 	return binary + " -tt";
diff --git a/src/support/os_win32.cpp b/src/support/os_win32.cpp
index e4f9ebd0fc..536d2a6446 100644
--- a/src/support/os_win32.cpp
+++ b/src/support/os_win32.cpp
@@ -229,8 +229,8 @@ void init(int argc, char ** argv[])
 			// to the outer split which sets cygdrive with its
 			// contents until the first blank, discarding the
 			// unneeded return value.
-			if (p.first != -1 && prefixIs(p.second, "Prefix"))
-				split(split(p.second, '\n'), cygdrive, ' ');
+			if (p.valid && prefixIs(p.result, "Prefix"))
+				split(split(p.result, '\n'), cygdrive, ' ');
 		}
 	}
 
-- 
2.28.0.windows.1



More information about the lyx-devel mailing list