search

PDP-10 / klh10

Community maintained version of Kenneth L. Harrenstien's PDP-10 emulator.
Other
59 stars 7 forks source link

some static analysis results #35

Open frink0 opened 6 years ago

frink0 commented 6 years ago

I decided to feed the source (well, the ITS build, for the moment) through the scan-build utility, and it did indeed turn up a bunch of issues, some of which are trivial patches (e.g. unused or uninitialized variables), others (use-after-frees, for example) that are probably best subject to the gaze of someone who has actually spent time with the code, and in any event I don't want to step on anyone's toes. Would it be best just to attach the results for people to pick over?

Rhialto commented 6 years ago

Yes, please do! We can then decide if we fix things in one big change of lots of small ones, or something.

frink0 commented 6 years ago

Certainly: ks-its-static-analysis-results.zip

Rhialto commented 6 years ago

Thanks! I am having a quick look at the reports, and some of them are indeed false alerts. It is very creative in finding execution paths, but it doesn't realise that the 4 cases in the switch in the very first report do indeed cover all possibilities, and there is no separate default case needed. Some other cases look more plausible though!

Rhialto commented 6 years ago

Here are some observations about some pseudo-randomly chosen reports. I label them with their final path component.

report-e6351b.html kn10cpu.c Assigned value is garbage or undefined

False positive. At step 33 / line 1178 there is no "default" branch since all possible cases are covered.

report-44bd54.html vmtape.c Assigned value is garbage or undefined

Yes.. At line 3049, nput always gets set IF len is not 0. Otherwise, if n != 0, If control does not pass that point, it must be because the if condition is true, and the function returns at line 3054. But in this scenario the function gets called with 0 at step 4 / line 2559. Conclusion: nput should be pre-set to 0 somewhere. Or should it be 1 to count this as an "empty frame" such as the caller comments?

report-b20600.html tapedd.c Assigned value is garbage or undefined

Yes. There should be a continue after swerror() like in various other cases. Or the assignment should be in the else part of the condition.

report-34b049.html kn10pag.c Branch condition evaluates to a garbage value

False positive. At step 9 / line 605 there is no "default" branch since all possible cases are covered.

report-2dedf0.html vmtape.c Use-after-free

This one is a bit more interesting. The function call at line 3931 free()s pd. In the next line, pd is compared to something. Strictly speaking, one can only compare pointers that point into the same object, and this object is no longer an object. However, that's not what the analyzer seems to claim. It seems to go round the loop once more (step 12 and 13), including another of the function calls. Only after that, it claims "use-after-free". I'm not sure how to see that (maybe it analyzed the data structure better than I did?) The solution for the potential problem I saw, would be that the loop condition rd != pd is calculated and stored before calling td_rddel().

ETA: I could see a NULL ptr reference in this loop, if there is only one element in the circular list. Then td_rddel() will set the pointer to the current element td->tdrd to NULL and the next time around the loop it segfaults. But having such a short list seems to be covered at line 3922.

That's it, so far.