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

[Pádraig Brady]

On 28/06/18 02:56, Kamil Dudka wrote:
> 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.

It's more subtle than that.
For most dev work, lint should be defined to avoid
mem leak check and static analysis false positives.
This is due to leak checker not knowing process is about to end,
and static analyzer limitations (often with cross module issues).
Ideally we'd not need to guard initializations like this at all.

On the other hand we do _not_ want define lint for ASAN runs.
I.E. we want to avoid default init for release builds due to
perf of redundant init and free().
For ASAN we want to avoid the redundant init so
that a UMR is actually flagged.  For example in this case
reading a zeroed stat would not be flagged by ASAN but
would be a logic error.
zeroing a stat buffer as init is always incorrect
and should not be in prod code.

cheers,
Pádraig