On Tue, Mar 22, 2022 at 08:47:26AM +0300, Dan Carpenter wrote: > On Mon, Mar 21, 2022 at 02:59:08PM -0700, Darrick J. Wong wrote: > > On Mon, Mar 21, 2022 at 10:33:02AM +0300, Dan Carpenter wrote: > > > b82670045aab66 Darrick J. Wong 2022-01-06 1365 > > > b82670045aab66 Darrick J. Wong 2022-01-06 1366 error = xfs_alloc_find_freesp(tp, pag, cursor, end_agbno, &len); > > > b82670045aab66 Darrick J. Wong 2022-01-06 1367 if (error) > > > b82670045aab66 Darrick J. Wong 2022-01-06 1368 goto out_cancel; > > > b82670045aab66 Darrick J. Wong 2022-01-06 1369 > > > b82670045aab66 Darrick J. Wong 2022-01-06 1370 /* Bail out if the cursor is beyond what we asked for. */ > > > b82670045aab66 Darrick J. Wong 2022-01-06 1371 if (*cursor >= end_agbno) > > > b82670045aab66 Darrick J. Wong 2022-01-06 @1372 goto out_cancel; > > > > > > This looks like it should have an error = -EINVAL; > > > > Nope. If xfs_alloc_find_freesp moves @cursor goes beyond end_agbno, we > > want to exit early so that the xfs_map_free_extent caller will return to > > userspace. > > > > --D > > I'm generally pretty happy with this static checker rule. Returning > success on a failure path almost always results if something bad like a > NULL deref or a use after free. But false positives are a real risk > because it's tempting to add an error code to this and introduce a bug. > > Smatch will not print the warning if error is set within 4 lines of the > goto. > error = 0; > if (*cursor >= end_agbno) > goto out_cancel; The trouble is, if I do that: error = xfs_alloc_find_freesp(...); if (error) goto out_cancel; error = 0; if (*cursor >= end_agbno) goto out_cancel; then I'll get patch reviewers and/or tools complaining about setting error to zero unnecessarily. Either way we end up with a lot of code golf for something the compiler will probably remove for us. Question: Can sparse detect that the if() test involves a comparison between a non-pointer function argument and a dereferenced pointer argument? Would that be sufficient to detect functions that advance a cursor passed in by the caller and return early when the cursor moves outside of a range also specified by the caller? --D > Another option is that people have started adding comments to these > blocks in response to the checker warning. > > Or if you had a different idea about how to silence the checker warning > I can also probably implement that. > > regards, > dan carpenter > >
