Hi!
Recently we've been getting a steady stream of patches from Changzhong
to fix missing assignment to error variables before jumping to error
cases.
I wonder if for new code it'd make sense to add an annotation for a type
which has to be returned non-zero?
What I have in mind is the following common flow:
int do_a_thing(struct my_obj *obj, int param)
{
int err;
err = first_step(obj, 1);
if (err)
return err;
if (some_check(obj)) {
err = -EINVAL; /* need explicit error set! */
goto err_undo_1s;
}
err = second_step(obj, param);
if (err)
goto err_undo_1s;
err = third_step(obj, 0);
if (err)
goto err_undo_2s;
return 0;
err_undo_2s:
second_undo(obj);
err_undo_1s:
first_undo(obj);
return err;
}
The variable err should never be returned when it's equal to 0.
So if we annotate it, let's say as:
int __nzret err;
could sparse then warn if we forgot to assign it after
"if (some_check(obj))"?
Am I the only one who thinks this would be a good idea?
On Sat, Dec 5, 2020 at 2:34 PM Jakub Kicinski <[email protected]> wrote:
>
> Am I the only one who thinks this would be a good idea?
I don't think it's likely to be very useful, because a very common
pattern is to not have that separate "return 0" in the middle, but
more along the lines of
err = first_step(obj, 1);
if (err)
return err;
if (some_check(obj)) {
err = -EINVAL; /* need explicit error set! */
goto err_undo_1s;
}
err = second_step(obj, param);
if (err)
goto err_undo_1s;
err = third_step(obj, 0);
err_undo_2s:
second_undo(obj);
err_undo_1s:
first_undo(obj);
return err;
iow, the "undo" parts are often done even for the success cases. This
is particularly true when those first steps are locking-related, and
the code always wants to unlock.
Sparse also doesn't really do any value analysis, so I suspect it
wouldn't be trivial to implement in sparse anyway.
Syntactically, I also think it's wrong to annotate the variable - I
think the place to annotate would be the return statement, and say
"must be negative" there. Kind of similar to having function arguments
annotated as "must not be NULL" (which sparse also doesn't do, but
some other checking tools do, and sparse can at least _parse_
"__nonnull" even if it ends up being ignored).
It's a bit similar to gcc's has a "returns_nonnull" function
attribute, but that one is not "per return", it's a global "this
function cannot return NULL" thing so that callers can then be
optimized and NULL checks removed. So it's very similar to the
"argument is non-null" in that it's for warnings at the *caller*, not
the function itself.
So if we want sparse support for this, I'd suggest something more akin
to a smarter compile-time assert, IOW more like having a
compile_time_assert(err < 0);
return err;
and then sparse (or any other checker) could warn when there's a path
that results in "err" not being negative.
Having some kind of smarter compile-time assert could be useful in
general, but as mentioned, sparse doesn't really do value range
propagation right now, so..
Luc, any reactions?
Linus
On Sat, Dec 05, 2020 at 03:10:15PM -0800, Linus Torvalds wrote:
> On Sat, Dec 5, 2020 at 2:34 PM Jakub Kicinski <[email protected]> wrote:
> >
> > Am I the only one who thinks this would be a good idea?
>
> err = third_step(obj, 0);
>
> err_undo_2s:
> second_undo(obj);
> err_undo_1s:
> first_undo(obj);
> return err;
>
> iow, the "undo" parts are often done even for the success cases. This
> is particularly true when those first steps are locking-related, and
> the code always wants to unlock.
>
> Sparse also doesn't really do any value analysis, so I suspect it
> wouldn't be trivial to implement in sparse anyway.
Yes but ... (see here under).
> Having some kind of smarter compile-time assert could be useful in
> general, but as mentioned, sparse doesn't really do value range
> propagation right now, so..
>
> Luc, any reactions?
I agree but the code Jakub showed is very constrained:
* only 2 return points
* one of them being 0, the other is to be checked.
and I think this should be checkable easily, something like:
* identify the highest point that can't reach the 'return 0'
* check that the only way to reach this point is via a zero/non-zero
test of the 'err' variable/returned value (which is a very
limited kind of value analysis after all).
But sure, these are rather strict constraints but maybe it's
common for net drivers?
Otherwise, yes, it's probably better to annotate the function itself
or the point of interest (via some kind of assertion) than the
variable.
I've not much idea how much this would be useful, though.
-- Luc
Hi Zhang,
Are you using Coccinelle to detect these bugs?
On Sat, Dec 05, 2020 at 02:32:50PM -0800, Jakub Kicinski wrote:
> Hi!
>
> Recently we've been getting a steady stream of patches from Changzhong
> to fix missing assignment to error variables before jumping to error
> cases.
I've mucked about with this a little in Smatch trying to work out some
heuristics to use. I added a warning for a NULL return followed by a
goto. Then on Friday I added a warning for a _dev_err() print followed
by a goto. But neither of those rules catches the bug fixed by commit
4de377b65903 ("net: marvell: prestera: Fix error return code in
prestera_port_create()"), where the error was invalid data.
if (idx >= size)
goto free_whatever;
I'm going to print a warning if the function ends in a cleanup block
that can only be reached by gotos. We'll see how that works tomorrow.
static void match_return(struct statement *stmt)
{
struct sm_state *sm, *tmp;
sval_t sval;
char *name;
bool is_last;
// Only complain if the function returns a variable
if (!stmt->ret_value || stmt->ret_value->type != EXPR_SYMBOL)
return;
// The function returns an int
if (cur_func_return_type() != &int_ctype)
return;
// It's only reachable via a goto
if (get_state(my_id, "path", NULL) != &label)
return;
// It returns a negative error code
sm = get_extra_sm_state(stmt->ret_value);
if (!sm || !estate_rl(sm->state) ||
!sval_is_negative(rl_min(estate_rl(sm->state))))
return;
FOR_EACH_PTR(sm->possible, tmp) {
// There is at least one path where "ret" is zero
if (estate_get_single_value(tmp->state, &sval) &&
sval.value == 0)
goto warn;
} END_FOR_EACH_PTR(tmp);
return;
warn:
// It's the last statement of a function
is_last = is_last_stmt(stmt);
name = expr_to_str(stmt->ret_value);
sm_warning("missing error code '%s' rl='%s' is_last=%d", name, sm->state->name, is_last);
free_string(name);
}
regards,
dan carpenter
On 2020/12/8 21:28, Dan Carpenter wrote:
> Hi Zhang,
>
> Are you using Coccinelle to detect these bugs?
In fact, I'm not familiar with Coccinelle, these bugs are reported by robot.
>
> On Sat, Dec 05, 2020 at 02:32:50PM -0800, Jakub Kicinski wrote:
>> Hi!
>>
>> Recently we've been getting a steady stream of patches from Changzhong
>> to fix missing assignment to error variables before jumping to error
>> cases.
>
> I've mucked about with this a little in Smatch trying to work out some
> heuristics to use. I added a warning for a NULL return followed by a
> goto. Then on Friday I added a warning for a _dev_err() print followed
> by a goto. But neither of those rules catches the bug fixed by commit
> 4de377b65903 ("net: marvell: prestera: Fix error return code in
> prestera_port_create()"), where the error was invalid data.
>
> if (idx >= size)
> goto free_whatever;
>
> I'm going to print a warning if the function ends in a cleanup block
> that can only be reached by gotos. We'll see how that works tomorrow.
>
> static void match_return(struct statement *stmt)
> {
> struct sm_state *sm, *tmp;
> sval_t sval;
> char *name;
> bool is_last;
>
> // Only complain if the function returns a variable
> if (!stmt->ret_value || stmt->ret_value->type != EXPR_SYMBOL)
> return;
>
> // The function returns an int
> if (cur_func_return_type() != &int_ctype)
> return;
>
> // It's only reachable via a goto
> if (get_state(my_id, "path", NULL) != &label)
> return;
>
> // It returns a negative error code
> sm = get_extra_sm_state(stmt->ret_value);
> if (!sm || !estate_rl(sm->state) ||
> !sval_is_negative(rl_min(estate_rl(sm->state))))
> return;
>
> FOR_EACH_PTR(sm->possible, tmp) {
> // There is at least one path where "ret" is zero
> if (estate_get_single_value(tmp->state, &sval) &&
> sval.value == 0)
> goto warn;
> } END_FOR_EACH_PTR(tmp);
>
> return;
> warn:
> // It's the last statement of a function
> is_last = is_last_stmt(stmt);
>
> name = expr_to_str(stmt->ret_value);
> sm_warning("missing error code '%s' rl='%s' is_last=%d", name, sm->state->name, is_last);
> free_string(name);
> }
>
> regards,
> dan carpenter
>
> .
>
I've pushed my Smatch check for missing error codes.
https://github.com/error27/smatch/commit/be18f90f05b684c12b80b9364b5bbc5dbef922da
I ended up writing a slightly more tricky version of the check because
there were some places that do:
ret = 0;
goto out;
And I didn't want to generate a warning for those. The heuristic is
that if "ret" is up to 3 lines before the goto then it's probably
intentional. There are still some false positives, especially in fs/
where "ret" is set to zero at the start of the function but it's
inentional.
I considered doing some more checking to say "this is an error path" but
I kind of like it as is. I have a separate unpublished check for
"this is an error path and there is a goto but the error code is not set"
and I will probably fix that up and publish it as well. So it will be
two warnings. :) Or vs And.
I've also attached the generated warnings from Friday's linux-next if
you want to take a look.
regards,
dan carpenter