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

[Kamil Dudka]

On Thursday, June 28, 2018 5:59:13 AM CEST Pádraig Brady wrote:
> On 27/06/18 06:36, Kamil Dudka wrote:
> > 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

Thanks for the analysis!

> Now we should be reading src_mode here which is already initialized if
> linting. That's done in the attached.

This helps only partially because src_sb is accessed by seen_file() and 
record_file() a few lines below.

> 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 memset() looks correct but I am not sure about the use of #ifdef lint.  
The lint macro can be useful to optimize out destruction of complex data 
structures on program exit.  Optimizing out initialization of a fixed-size
local variable with scope over 1856 lines in some I/O intensive code is 
probably not worth the troubles.

> 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.

Thanks for the confirmation!

> Note one should define 'lint' when running static analysis,
> to avoid false positives.

Yes and no.  Because, if you do, you will analyze different code than you run 
in production builds.  You might be able to prove correctness of the code now 
but, if you mute static analyzers by adding IF_LINT(=0) and passing -Dlint to 
preprocessor, you will not be notified later on when some tiny change in the 
copy_internal() function (or even in the calling context) causes a real use
of uninitialized contents of stack.

Kamil

> The static analysis is much appreciated.
> 
> thanks!
> Pádraig