Re: [PATCH v6 11/27] revisions API users: add "goto cleanup" for release_revisions()
From: Jeff King <hidden>
Date: 2022-07-11 18:06:42
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Wed, Apr 13, 2022 at 10:01:40PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted hunk ↗ jump to hunk
diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 70103c40952..2bfaf9ba7ae 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c@@ -77,8 +77,12 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) if (read_cache_preload(&rev.diffopt.pathspec) < 0) { perror("read_cache_preload"); - return -1; + result = -1; + goto cleanup; } +cleanup: result = run_diff_files(&rev, options); - return diff_result_code(&rev.diffopt, result); + result = diff_result_code(&rev.diffopt, result); + release_revisions(&rev); + return result; }
A bit late, but I happened to notice Coverity complaining about this code. And indeed, this patch seems pretty broken. If read_cache_preload() fails, we assign "-1" to result and jump to cleanup. But then the first thing we do in cleanup is overwrite result! That hides the error (depending on how run_diff_files behaves if the cache load failed, but one can imagine it thinks there are no files to diff). Should the cleanup label come after the call to run_diff_files()? I was also somewhat confused by the double-assignment of "result" in the cleanup label. But I think that is because diff_result_code() is massaging the current value of "result" into the right thing. But in that case, should the "-1" from earlier be passed to diff_result_code()? I think probably not (and certainly it was not before your patch). Which would imply that the label should go after that, like:
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 2bfaf9ba7a..92cf6e1e92 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c@@ -80,9 +80,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) result = -1; goto cleanup; } -cleanup: result = run_diff_files(&rev, options); result = diff_result_code(&rev.diffopt, result); +cleanup: release_revisions(&rev); return result; } -Peff