Re: New Defects reported by Coverity Scan for ceph

[Brad Hubbard <bhubbard@redhat.com>]

On Thu, Dec 21, 2017 at 1:34 PM, Jos Collin <jcollin@redhat.com> wrote:
>
>
> On Thursday 21 December 2017 07:24 AM, scan-admin@coverity.com wrote:
>>
>>
>> Hi,
>>
>> Please find the latest report on new defect(s) introduced to ceph found
>> with Coverity Scan.
>>
>> 3 new defect(s) introduced to ceph found with Coverity Scan.
>> 11 defect(s), reported by Coverity Scan earlier, were marked fixed in the
>> recent build analyzed by Coverity Scan.
>>
>> New defect(s) Reported-by: Coverity Scan
>> Showing 3 of 3 defect(s)
>>
>>
>> ** CID 1426746:  Null pointer dereferences  (REVERSE_INULL)
>> /home/brad/working/src/ceph/src/mds/MDBalancer.cc: 1270 in
>> MDBalancer::dump_loads(ceph::Formatter *)()
>>
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1426746:  Null pointer dereferences  (REVERSE_INULL)
>> /home/brad/working/src/ceph/src/mds/MDBalancer.cc: 1270 in
>> MDBalancer::dump_loads(ceph::Formatter *)()
>> 1264
>> 1265       f->open_array_section("dirfrags");
>> 1266       while (!dfs.empty()) {
>> 1267         CDir *dir = dfs.front();
>> 1268         dfs.pop_front();
>> 1269
>>>>>
>>>>>      CID 1426746:  Null pointer dereferences  (REVERSE_INULL)
>>>>>      Null-checking "f" suggests that it may be null, but it has already
>>>>> been dereferenced on all paths leading to the check.
>>
>> 1270         if (f) {

This check for f being null is redundant since if it were null we
would have segfaulted on line 1263 or 1265.

I'd suggest it can be removed. We could put an assert put at the start
of the function if necessary/desirable  to check the Formatter
argument being passed in but I haven't looked into it that deeply and
I'm not sure that's necessary. I'd also suggest taking a look at
similar functions that take a "Formatter" argument and see how they
handle the possibility of the Formatter being null.

>> 1271           f->open_object_section("dir");
>> 1272           dir->dump_load(f, now, decayrate);
>> 1273           f->close_section();
>> 1274         }
>> 1275
>>
>> ** CID 1426747:  Null pointer dereferences  (FORWARD_NULL)
>> /home/brad/working/src/ceph/src/mds/MDBalancer.cc: 1290 in
>> MDBalancer::dump_loads(ceph::Formatter *)()
>
>
> There is a possibility that `f` might be NULL. But it is not NULL.
>
> Please see:
> https://github.com/ceph/ceph/blob/f33ab7e03a13e18c8c883284033d511f1b43df12/src/mds/MDSDaemon.cc#L135
>
>>
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1426747:  Null pointer dereferences  (FORWARD_NULL)
>> /home/brad/working/src/ceph/src/mds/MDBalancer.cc: 1290 in
>> MDBalancer::dump_loads(ceph::Formatter *)()
>> 1284            if (subdir->pop_nested.meta_load() < .001)
>> 1285              continue;
>> 1286            dfs.push_back(subdir);
>> 1287           }
>> 1288         }
>> 1289       }
>>>>>
>>>>>      CID 1426747:  Null pointer dereferences  (FORWARD_NULL)
>>>>>      Passing null pointer "f" to "close_section", which dereferences
>>>>> it. (The dereference happens because this is a virtual function call.)
>>
>> 1290       f->close_section();  // dirfrags array
>> 1291
>> 1292       f->open_object_section("mds_load");
>> 1293       {
>> 1294
>> 1295         auto dump_mds_load = [this, f, now](mds_load_t& load) {
>>
>> ** CID 1426748:  Uninitialized members  (UNINIT_CTOR)
>> /home/brad/working/src/ceph/src/osd/ECBackend.h: 511 in
>> ECBackend::Op::Op()()
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Cheers,
Brad