View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0005150unrealircdpublic2018-09-25 10:552018-09-26 03:29
Reportershenlanting Assigned To 
PrioritynormalSeveritytweakReproducibilityhave not tried
Status acknowledgedResolutionopen 
Summary0005150: a deference of a null pointer or a redundant check against null?
DescriptionHi all,
There is a deference of a null pointer or a redundant check against null found by Qihoo360 CodeSafe Team.

Details as bellow:
In file 'src/modules/blacklist.c', line 125, the expand of macro 'MARK_AS_OFFICIAL_MODULE' will check whether the argument 'modinfo' is null or not.
The the calling for function 'ModuleSetOptions()' (line 126) just uses the deference of 'modinfo' (by using 'modinfo->handle') without checking against null.

Should we add a check against null here or is the checking against null above is redundant?
TagsQihoo360 CodeSafe Static Analysis
3rd party modules

Activities

syzop

2018-09-25 11:28

administrator   ~0020345

Hi,

Thanks for your interest in checking and reporting issues in our source code.

That being said, it looks like you are reporting all the issues a (static) code analyzer is reporting. Could you first try to look into it, eg: try to reproduce the issue, before reporting it?
Many issues reported by static code analyzers are not an issue, this likely includes all 3 issues you reported so far. I know this first hand, as we use a static code analyzer as well and I had to suppress about a hundred issues.

That being said, if you do find something you can actually (re)produce, then I'm always very interested to hear about such issues.

Regards,

Bram

shenlanting

2018-09-25 11:53

reporter   ~0020348

Thanks for your reply.
In fact, I've already look into every item I reported which is also asked in our group.

So, in this item, the check in macro 'MARK_AS_OFFICIAL_MODULE' is just redundant here, but may be useful in other place?
In this situation, we still suggest a new macro or just a statement without checking against here if modinfo will never be a null pointer.

A better coding style (the macro is expanded):

//MARK_AS_OFFICIAL_MODULE(modinfo);
//#define MARK_AS_OFFICIAL_MODULE(modinf)
do {
  if (modinf && modinf->handle)
    ModuleSetOptions(modinfo->handle, MOD_OPT_OFFICIAL, 1);
} while (0)
if (modinfo)
  ModuleSetOptions(modinfo->handle, MOD_OPT_PERM, 1);

or:
//MARK_AS_OFFICIAL_MODULE(modinfo);
//#define MARK_AS_OFFICIAL_MODULE(modinf)
do {
  ModuleSetOptions(modinfo->handle, MOD_OPT_OFFICIAL, 1);
} while (0)
ModuleSetOptions(modinfo->handle, MOD_OPT_PERM, 1);


-----------------

by the way, about the difinition of the macro 'MARK_AS_OFFICIAL_MODULE' in line 1761 in file 'struct.h' really confuses us:

#define MARK_AS_OFFICIAL_MODULE(modinf) do { if (modinf && modinf->handle) ModuleSetOptions(modinfo->handle, MOD_OPT_OFFICIAL, 1); } while(0)

the parameter of this macro is 'modinf', which is checked in the if statement in the expand of the macro. But when calling function 'ModuleSetOptions()', the argument is 'modinfo'. Are these really different virables or is it just a typo?

syzop

2018-09-25 13:52

administrator   ~0020350

The reports come across as rather 'unfiltered'. So if you already filtered them to cut the noise, that is good and I thank you for that :).
But.. maybe you can filter a bit more.. I'm really only interested in issues that actually occur in the software. As said, I have some experience with code analyzers - such as Coverity - and they can be a useful tool. Unfortunately the 'noise' to 'real bugs' ratio is often very bad... with lot's of things that don't apply in practice. You have to go through them one by one to verify there is a real issue. The config "test" and "run" stuff from the other report is a notorious one that static code analyzers do not understand. The analyzer does not understand there is no issue.

I'm interested in: bugs that actually exist/occur when running the IRCd

Not interested in: unused variables, crashes when out of memory, issues that will never occur (because of other checks in the rest of the code, etc.), ..

So yeah, if you could try to report only the 'real bugs' then they are welcome. If there will be a lot of noise then I think I'll quickly get tired/annoyed by the time spent on non-relevant bugs. And you probably won't be very happy either :).

By the way, I am saying this so early because you are probably looking at 100-1000 "reports" from the static code analyzer, and I really don't want you to report 100 "bugs" to us without proper checking. That is why I am so quick with my response. If my reaction is offensive then my apologies, I'm not sure how to say it in a different way. In my view the researcher (you?) is the one that makes the difference when interpreting static code analyzer. They are the ones that should check if an issue is real or not. And yes, that usually takes a lot of time :)

**
Then about this specific bug report:
Turns out this is actually about a typo, where in the macro definition 'modinfo' should be 'modinf'. Yes, sure I'll fix that.
I think MARK_AS_OFFICIAL_MODULE() is called by all code as MARK_AS_OFFICIAL_MODULE(modinfo) so hence there is no crash issue anywhere, but it is indeed a typo that needs to be fixed.

shenlanting

2018-09-26 03:29

reporter   ~0020354

Thanks again for you response.
There is no 'offensive' at all, and no need to worry about it.
But if my report bothers you, I apologize.

The checking style of our tool is not noise kind, but, yes, sill not exactly issues that will occur. Static analysis tool to report such issues is still experimental (some tool may use fuzzing test which will cost more time and equipment, and this kind of test is actually not static analysis).

If the code may be used (such as copy to) in another project, a very different (unknown) situation, or the environment (such as data input) may be very complex, the report could be more useful.

Won't bother you again.

Regards,
Lanting.

Issue History

Date Modified Username Field Change
2018-09-25 10:55 shenlanting New Issue
2018-09-25 10:55 shenlanting Tag Attached: Qihoo360 CodeSafe Static Analysis
2018-09-25 11:28 syzop Note Added: 0020345
2018-09-25 11:53 shenlanting Note Added: 0020348
2018-09-25 13:52 syzop Note Added: 0020350
2018-09-25 13:54 syzop Status new => acknowledged
2018-09-26 03:29 shenlanting Note Added: 0020354