sosy-lab / sv-benchmarks

Collection of Verification Tasks (MOVED, please follow the link)
https://gitlab.com/sosy-lab/benchmarking/sv-benchmarks
184 stars 169 forks source link

Unreach-call verdict of ldv-linux-4.2-rc1/linux-4.2-rc1.tar.xz-08_1a-drivers--media--dvb-core--dvb-core.ko-entry_point.cil.out.yml #1207

Open sim642 opened 3 years ago

sim642 commented 3 years ago

In our own testing Goblint gave the result true for ldv-linux-4.2-rc1/linux-4.2-rc1.tar.xz-08_1a-drivers--media--dvb-core--dvb-core.ko-entry_point.cil.out.yml but its expected verdict is false. Obviously we want to fix our unsoundness so I've been desperately trying to understand this program and how it can reach the error. Unfortunately I haven't been successful so I'm asking about it here to avoid going truly insane: is there really a violation or could the expected verdict be itself wrong?

Below are a few things which have made me suspect the latter.

Previous SV-COMPs

I looked through the results of previous SV-COMPs which included this benchmark and I couldn't find any tool to have solved this correctly, so no violation witness to inspect either. There were only two cases ever where a tool gave a non-timeout non-error result:

Goblint's result

Goblint's true output also comes with a correctness witness witness.txt (renamed to .txt for GitHub). Unfortunately I haven't been able to validate it. I tried both CPAchecker and Ultimate (without time limit and 20GB of memory) and both eventually ran out of that memory.

What Goblint's own output says more directly is that supposedly large parts of this program are dead code.

Manual analysis

Finally I turned myself into a verifier in order to try to understand the program and its supposed violation. Working backwards from error locations I managed to convince myself of the same thing: all the ways I could see for reaching the error are dead code due to how some state variables are initialized and manipulated.

For reference, below is a rough dump of my internal state that lead me to that conclusion.

My manual backwards analysis notes

PhilippWendler commented 3 years ago

@mutilin?

mutilin commented 3 years ago

@sim642 Simmo, the intention of the benchmark was to demonstrate a violation of Module get/put rule. In this example there is a mismatch between try_module_get expanded from fops_get and module_put from fops_replace in dvb_device_open.

As you correctly wrote, current example misses some initialization of dvb_minors, which should be done by dvb_register_device from dvb_dmxdev_init. Alternatively, it may be initialized by arbitrary values.

sim642 commented 3 years ago

@mutilin Since you know the structure of these benchmarks better than I do, could you open a PR to put the missing initialization to some appropriate place which would restore the original intent of the benchmark?

I assume that would be preferable to changing the expected verdict to true, which would match the existing benchmark code but misses the original intent.

mutilin commented 3 years ago

I think it is better to move it to some TODO directory or remove, because now I do not have time to fix it.