Re: new snapshot available: coreutils-8.29.64-1755f.tar.xz

[Pádraig Brady]

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

copy-static-warnings.patch
Description: Text Data