On 27/06/18 06:36, Kamil Dudka wrote:
> On Wednesday, June 27, 2018 12:02:07 PM CEST Pádraig Brady wrote:
>> We plan to release coreutils-8.30 in the coming week
>> so any testing you can do on various different systems between now and then
>> would be most welcome.
>>
>> --------------------------------------
>>
>> You can download the coreutils snapshot in xz format (5.3 MB) from:
>> https://pixelbeat.org/cu/coreutils-ss.tar.xz
>>
>> And verify with gpg or md5sum with:
>> https://pixelbeat.org/cu/coreutils-ss.tar.xz.sig
>> MD5 (coreutils-ss.tar.xz) = 3ce92d2c8da2842328415dab8d1f4bbc
>
> I am getting many warnings from static analyzers about uses of uninitialized
> stat buffer in the copy_internal() function. I think it is related to the
> optimization of stat() calls implemented by Paul recently.
>
> If you think that the reported code paths are not feasible, please make
> the buffer at least explicitly initialized to make sure that it behaves
> predictably in all corner cases.
>
> Details attached.
I had run the tests with ASAN which noticed no issues.
The central issue you reported is:
Error: UNINIT (CWE-457):
coreutils-8.29.64-1755f/src/copy.c:1856: var_decl: Declaring variable "src_sb"
without initializer.
coreutils-8.29.64-1755f/src/copy.c:1873: cond_true: Condition "x->move_mode",
taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1875: cond_true: Condition "rename_errno <
0", taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1876: cond_false: Condition "renameat2(-100,
src_name, -100, dst_name, 1U /* 1 << 0 */)", taking false branch.
coreutils-8.29.64-1755f/src/copy.c:1879: cond_true: Condition "rename_errno ==
0", taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1879: cond_true: Condition "rename_errno ==
0", taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1880: cond_true: Condition
"rename_succeeded", taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1884: cond_true: Condition "rename_errno ==
0", taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1884: cond_false: Condition "!x->last_file",
taking false branch.
coreutils-8.29.64-1755f/src/copy.c:1884: cond_false: Condition "(rename_errno
== 0) ? !x->last_file : (rename_errno != 17 || x->interactive != I_ALWAYS_NO)",
taking false branch.
coreutils-8.29.64-1755f/src/copy.c:1905: if_end: End of if statement.
coreutils-8.29.64-1755f/src/copy.c:1911: cond_true: Condition
"command_line_arg", taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1911: cond_true: Condition "x->src_info",
taking true branch.
coreutils-8.29.64-1755f/src/copy.c:1913: uninit_use: Using uninitialized value
"src_sb.st_mode".
# 1911| if (command_line_arg && x->src_info)
# 1912| {
# 1913|-> if ( ! S_ISDIR (src_sb.st_mode)
# 1914| && x->backup_type == no_backups
# 1915| && seen_file (x->src_info, src_name, &src_sb))
This looks ok as:
x->src_info only set in cp, so stat buffer only read here in cp
x->rename_errno only set in mv, so stat always done in cp/install
Now we should be reading src_mode here which is already initialized if linting.
That's done in the attached. Also done in the attached is to explicitly
call init src_sb if !x->move_mode and linting, which is clearer to readers and
analyzers.
The next significant issue (only considering mv from here) is:
Error: CLANG_WARNING:
coreutils-8.29.64-1755f/src/copy.c:1977:29: warning: The left operand of '&' is
a garbage value
# if (x->update && !S_ISDIR (src_mode))
src_mode always set when x->update as mv -u is ignored with -n,
and only with -n is src_mode not set.
The next potential mv issue is:
coreutils-8.29.64-1755f/src/copy.c:2261:25: note: Left side of '&&' is false
# else if (x->recursive && S_ISDIR (src_mode))
However we're guaranteed not to get to that without src_mode set
as we abandon_move() unconditionally earlier due to -n,
and without -n we'd have set src_mode.
So the logic looks sound.
Note one should define 'lint' when running static analysis,
to avoid false positives.
The static analysis is much appreciated.
thanks!
Pádraig
From 4d1d6ff64f71483a0cc00cffb2ce1b4c359a03de Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Wed, 27 Jun 2018 20:33:30 -0700
Subject: [PATCH] maint: copy: avoid new static analyzer warnings
* src/copy.c (copy_internal): Use the lint protected src_mode,
rather than accessing the src_sb again. Also unconditionally
populate src_sb when !x->move_mode and in lint mode.
Reported by Kamil Dudka with coverity and clang analyzer.
---
src/copy.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/copy.c b/src/copy.c
index eccf67c..58d2f6e 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1903,6 +1903,13 @@ copy_internal (char const *src_name, char const *dst_name,
return false;
}
}
+#ifdef lint
+ else
+ {
+ assert (x->move_mode);
+ memset (&src_sb, 0, sizeof src_sb);
+ }
+#endif
/* Detect the case in which the same source file appears more than
once on the command line and no backup option has been selected.
@@ -1910,7 +1917,7 @@ copy_internal (char const *src_name, char const *dst_name,
This check is enabled only if x->src_info is non-NULL. */
if (command_line_arg && x->src_info)
{
- if ( ! S_ISDIR (src_sb.st_mode)
+ if ( ! S_ISDIR (src_mode)
&& x->backup_type == no_backups
&& seen_file (x->src_info, src_name, &src_sb))
{
--
2.9.3
