search

jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.22k stars 1.74k forks source link

Intentional segfault never occurs in tests sodium_utils{23} #1390

Closed byron-hawkins closed 3 months ago

byron-hawkins commented 3 months ago

In file <libsodium>/test/default/sodium_utils2.exp, the following lines are expected to be generated by a successful execution of the test:

OK
Intentional segfault / bus error caught
OK

On both linux x86_64 and android aarch64, nothing like this occurs. When I run the test in gdb, it does not segfault, but rather completes successfully and without any error or any output of any kind. Same situation for sodium_utils3.exp. Is this a problem with my build, or are these .exp files possibly obsolete? Flag -DBENCHMARKS=1 is not set for this build (and no timer output appears).

It also seems mysterious that the comparison loop in main() of <libsodium>/test/default/cmptest.h does not identify the discrepancy. Perhaps the length of .res should be compared with the length of .exp before looping the content? An especially messy scenario would be an incorrect .res file that is longer than its corresponding .exp file, in which case fgetc() would blindly go beyond EOF on the .exp file.

jedisct1 commented 3 months ago

This is expected. Since f1da14de2b36a3e142ba1ae2f992ebb0632be473 we call _exit() if the segmentation fault was properly trapped. Going any further would not be safe in the context of a signal handler, and even the printf() statements are borderline.

Instead, if the signal is NOT caught, the main() function goes on, outputs Overflow not caught , and the regular comparison between the expected and actual results will fail.

Ironically, Intentional segfault / bus error caught in the .exp file is there for clarity, in order to explain what we expect here, and hopefully not have people open the issue you just did :)

byron-hawkins commented 3 months ago

There is still a problem in the case that a test may stop short. For example, if there are 4 lines in the .res file and 8 lines in the .exp file, and if the initial 4 lines match, then the test will pass, even though it quit halfway through. Can the file sizes be checked before comparing?

jedisct1 commented 3 months ago

Sure, it doesn't hurt to check that we don't have additional lines, even empty ones in .exp files. 1d8e308b9d517d55c96a3953e104c49f68489c87 should do that.

jedisct1 commented 3 months ago

Wait, no, this is not required. We already return 99 if the content doesn't match, including the final EOF.

byron-hawkins commented 3 months ago

If that were the case, then the sodium_utils{23} tests would fail, because the .res files most definitely do not match the .exp files provided. But those tests are passing, so the file comparison is not precise.

jedisct1 commented 3 months ago

They test that we didn't get Overflow not caught nor Underflow not caught. That's the purpose of these tests.

byron-hawkins commented 3 months ago

There remains a problem in the case that a test may exit before completing all of its steps. As I explained before, if the .res file contains only 4 lines that match the first 4 lines of a longer .exp file, the test driver will not be aware that the test has not completed. As a workaround, I'm using diff test.res test.exp after every test, since the driver cmptest.h is unable to detect all important failure conditions.