Problem : Assigned value is garbage or undefined @ 12526.
Diagnostic : Multithreading issue.
Rationale : Error only occurs if global has_mbyte is modified while
function is executing.
Resolution : Use local copy of global.
Problem : Dereference of null pointer @ 6089.
Diagnostic : False positive / Real issue.
Rationale : From the code, it seems the intent is that len parameter
should never exceed SBLOCKSIZE. But the code checking for
that does in fact cause a null pointer dereference just
immediately after.
Resolution : State precondition in doc and assert it at entry.
Problem : Result of operation is garbage or undefined @ 5809.
Diagnostic : Real issue.
Rationale : When copying flags, first access to
`use_pfxlist[use_pfxlen]` was garbage if
`spin->si_compflags` was null.
Resolution : Make sure `use_pfxlist[use_pfxlen]` always has a value (NUL
if `spin->si_compflags` is NULL).
Problem : Uninitialized argument value @ 4469.
Diagnostic : Real issue.
Rationale : Happens when a line contains a spell info item (NAME, HOME,
VERSION, AUTHOR, EMAIL, COPYRIGHT), which expect a second
item, but then the second item is not present.
Resolution : Add guard (item count > 1) to failing branch.
Problem : Result of operation is garbage or undefined @ 2238.
Diagnostic : Real issue.
Rationale : Problem occurs when searching forward starting on an empty
line. This is, at 2127:
```
p = buf + skip;
endp = buf + len;
while (p < endp) {
```
when skip == 0, len == 0, implying p == endp and therefore
not entering the loop.
Under those conditions, comparison
```
if (attr == HLF_COUNT)
```
at line 2242 is really using a garbage value for `attr`.
Most of the time the error doesn't produce visible problems
as it only affects when dealing with wrapped words.
Resolution : Initialize `attr` at declaration to `HLF_COUNT`, which is
used in the code when no bad word found yet.
Problem : Argument with 'nonnull' attribute passed null @ 2118.
Diagnostic : False positive.
Rationale : Error happens when `if (buflen < len + MAXWLEN + 2) {` is
not entered on the first iteration, which cannot happen
because buflen is 0 on the first iteration, so the
condition should always hold.
Resolution : Assert existence of buffer with appropiate length after
conditional (which prevents previous error path).
Problem : Dead assignment @ 1602.
Diagnostic : Harmless issue.
Rationale : Code using this assignment (line 1666) was disabled. Vim's
tip at Wed Nov 12 13:07:54 2014 +0100 (changeset
6352:2f7bf5f90f57) hasn't changed this yet.
Resolution : Disable assignment. Directive processors are used for that
in order to match the way the other code was disabled.
Problem : Dereference of null pointer @ 4395.
Diagnostic : Multithreading issue.
Rationale : Problem occurs only if global g_do_tagpreview changed while
funcion is executing.
Resolution : Use local copy of global var.
Problem : Dead assignment @ 1554.
Diagnostic : Harmless issue.
Rationale : `result` is used when analyzing if a bracketed expresion
`[<whatever>]` can be condensed into a character class. Not
used for anything else anywhere. So, it's safe to remove.
Resolution : Remove dead assingment and move declaration of `result` to
the scope where it's used.
Problem : Dereference of null pointer @ 921.
Diagnostic : False positive.
Rationale : If `qi->qf_lists[qi->qf_curlist].qf_count == 0` doesn't
hold, we should be calling function with nonnull `*prevp`.
Resolution : Assert nonnull.
Problem : Dead assignment @ 2566.
Diagnostic : Harmless issue.
Rationale : `nextchar` is used as a lookahead buffer for the character
next to the currently examined token. Sometimes it also
saves that char while original string is modified (original
position of nextchar is nullified for the string to
terminate there). In summary, it's an auxiliary variable
with no particular complex meaning. Safe to remove if not
used.
Resolution : Remove dead assignment.
Problem: After splitting a window and setting 'breakindent' the default
minimum with is not respected.
Solution: Call briopt_check() when copying options to a new window.
https://code.google.com/p/vim/source/detail?r=v7-4-417
Return bool from checkclearop, checkclearopq, add_to_showcmd,
find_decl, nv_screengo, get_visual_text, and unadjust_for_sel as these
functions all return either TRUE or FALSE or OK or FAIL, but not MAYBE.
Change to arguments toplevel of normal_cmd, check of
do_check_scrollbind, and locally and thisblock of finddecl, to bools as
they represent predicates.
In do_pending_operator(), nv_zet(), nv_indent(), nv_g_cmd(), and more,
replace integer flags with bools when appropriate.
However, do keep the TRUEs and FALSEs of nv_cmds[].cmd_arg. It may store
values other than TRUE or FALSE, so it seems to make sense to use
integral constants.
Several opart_T members like use_reg_one, end_adjusted, empty,
is_VIsual, and block_mode, only ever store TRUE or FALSE, so make this
constraint explicit by changing them to bools, and TRUE to true and
FALSE to false in the context of their uses.
The member, inclusive, has several other uses such as in arithmetic
equations and one inequality, but every single assignment (obtained with
'grep -r "inclusive \\="') sets it to either TRUE or FALSE.
This also implies that the inequality, "oap->end.coladd <
oap->inclusive", can only be true when coladd==0 and inclusive==true, so
test for that instead.
For consistency, change the first argument of findpar (which ends up
being inclusive) to bool.
Include stdbool.h for consistency with issue #918.
This commit shrinks the size of oparg_T from 128 bytes to 112 (-13%) on
my machine.
Problem : Dead assignment @ 3323.
Dead assignment @ 3587.
Diagnostic : Harmless issues.
Rationale : - 3323: Assignment is in fact dead. But, in addition to
that, `length_modifier` is assigned default value `\0`
when declared and is untouched in path leading to
signaled point. So, maintaining assignment adds nothing
to code.
- 3587: Assignment is in fact dead. It could be thought
that `precision_specified` has to be 1 in order to flag
`precision` as having a valid value. But that doesn't
seem to be the case, as there are places in the code
where `precision` gets assigned a default value, even if
`precision_specified` is 0. So, maintaining assignment
adds nothing to code.
Resolution : Remove dead assignments.
Problem : Dereference of null pointer @ 693.
Diagnostic : False positive.
Rationale : Error condition occurs if `delete_first_msg` is entered two
consecutive times, the firt of which sets leaves history
empty. But, in that case, second entrance should leave at
the `return FAIL`, and thus cannot reach the pointer
dereference.
Resolution : Assert history will be empty after first entrance.
Problem : Undefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
@ 148.
Diagnostic : Cautionary warning.
Rationale : Reported circumstance (calling calloc with requesting 0
size allocation) can occur, and it's not an issue. It's
perfectly legal doing so, though result is implementation
dependant. A given implementation can return NULL or a
valid pointer, so that free() can be called on it later the
same as if it was a real pointer. Such a pointer should not
be dereferenced, though.
Now, for some reason I can't explain, compiler is warning
us in the case of calloc, but not in the case of malloc,
which is doing the same.
Resolution : Refactor memory functions to avoid using implementation
dependant behaviour.
Proposed code is neater to read, and it avoids calling
system memory functions with size 0, thus behaving the same
everywhere.
Note that semantics for xmalloc/xcalloc is slightly
changed:
- Previously, an implementation that returns a valid
pointer on malloc/calloc with 0 size, would return that
pointer to xmalloc/xcalloc caller.
- Currently, a regular pointer is always returned.
Problem : Bad free @ 3058.
Diagnostic : False positive uncovering a real issue.
Rationale : Signaled error occurs if p gets assigned `(char_u*)""` at
line 3009 and then is freed at line 3058. But that cannot
happen because of the last guard condition before `free`
(`*p != NUL`). So, signaled error is a false positive.
Now, examining this code more carefully reveals a real
issue: not freeing an empty string may not be always
correct, as an empty (but allocated) string could also be
returned in `p = string_convert(&prt_conv, p, &len);` if
passed '&len' points to 0). Which would in fact be a memory
leak.
Resolution : Remove the exceptional case. Make p always point to
allocated memory, and always free it, when `prt_do_conv` is
on.
Problem : Dereference of null pointer @ 6219.
Diagnostic : False positive.
Rationale : Problem occurs if `aucmd_win` is NULL after
`win_alloc_aucmd_win()`, which cannot happen since it uses
new memory functions. So, this is a leftover since OOM
refactorization.
Resolution : Remove dead code.
Problem : Dead assignment @ 1754.
Diagnostic : Harmless issue.
Rationale : It's true `iconv_fd` is not going to be used again (we are
in the failure handler). But what is being done (assigning
sentinel value to mark as "empty" after destroying) is in
fact good practice, which could turn significant if more
code is added later on. So, we don't want to remove this.
Resolution : Leave it there, but exclude from analysis.
Problem : Dereference of null pointer @ 1701.
Diagnostic : False positive.
Rationale : Comparison `last_wp != wp` just after initializing
`last_wp` to NULL makes the compiler think `wp` can be
null. Error appears then on codepath assuming comparison is
false (i.e. `wp` is null).
Resolution : Change order of OR clauses.
That seems not to give motives for the analyzer to check
the `wp` null path and removes the warning.
But potential null dereference is still there, so we add
the nonnull annotation to `wp` parameter.
Problem : Double free @ 4249.
Diagnostic : False positive.
Rationale : Codepath leading to error contains two consecutive
iterations in which `if (--j < 0)` is true.
That executes `free` two consecutive times with the same
value (hislen - 1) for j, with leads to double free.
Now, that can only happen with j == 0 && hislen == 1.
And that would imply j == hisidx[type] too, which would
take the following break.
So, the error codepath cannot really happen, but the
compiler cannot deduce the last implication.
Resolution : We have two possible solutions for this:
1.- Comparing value of j before and after updating it,
and breaking out of iteration if equal.
That changes nothing in functionality, but teaches the
compiler his proposed error codepath is impossible.
2.- Nullify pointer after freeing.
This way, the compiler still thinks his error codepath
is possible, but it's not an error anymore, as
free(NULL) is a no-op.
We opt for solution 2, as solution 1 requires adding
logic that adds nothing (and having to explain that clearly
in aside comments) just for the purpose of silencing
warning. On the other hand, solution 2 improves the code,
adding something considered good practice in any case,
and therefore doesn't require further explanation.
Problem : Dereference of null pointer @ 711.
Diagnostic : False positive.
Rationale : Codepath producing error invokes this function with values
`action=RPC_DISCARD, pending=CSTP_FINISH, value=NULL`.
Now, for some reason, the analyzer is remembering that
`value` is null, and that `action` is `RPC_DISCARD`, but
it's not remembering that `pending` is `CSTP_FINISH`.
Then, it's taking the wrong branch in the switch for
`pending`. That path would never occur invocating the
function with those values.
Resolution : Assert function precondition between `pending` and `value`.
This is, let the compiler know that `value` being null
implies `pending` not containing `CSTP_THROW`.
Problem : Uninitialized argument value @ 7704.
Diagnostic : False positive.
Rationale : Error occurs if `switch(spec_idx)` doesn't enter any case,
which should not occur after
`spec_idx = find_cmdline_var(...)` returned non-negative.
Resolution : Add default clause to switch and error if reached.
