Clam 34 unit test overhaul windows support #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
An ENABLE_TESTS CMake option is provided so that users can disable testing if they don't want it. Instructions for how to use this included in the INSTALL.cmake.md file. If you run `ctest`, each testcase will write out a log file to the <build>/unit_tests directory. As with Autotools' make check, the test files are from test/.split and unit_tests/.split files, but for CMake these are generated at build time instead of at test time. On Posix systems, sets the LD_LIBRARY_PATH so that ClamAV-compiled libraries can be loaded when running tests. On Windows systems, CTest will identify and collect all library dependencies and assemble a temporarily install under the build/unit_tests directory so that the libraries can be loaded when running tests. The same feature is used on Windows when using CMake to install to collect all DLL dependencies so that users don't have to install them manually afterwards. Each of the CTest tests are run using a custom wrapper around Python's unittest framework, which is also responsible for finding and inserting valgrind into the valgrind tests on Posix systems. Unlike with Autotools, the CMake CTest Valgrind-tests are enabled by default, if Valgrind can be found. There's no need to set VG=1. CTest's memcheck module is NOT supported, because we use Python to orchestrate our tests. Added a bunch of Windows compatibility changes to the unit tests. These were primarily changing / to PATHSEP and making adjustments to use Win32 C headers and ifdef out the POSIX ones which aren't available on Windows. Also disabled a bunch of tests on Win32 that don't work on Windows, notably the mmap ones and FD-passing (i.e. FILEDES) ones. Add JSON_C_HAVE_INTTYPES_H definition to clamav-config.h to eliminate warnings on Windows where json.h is included after inttypes.h because json-c's inttypes replacement relies on it. This is a it of a hack and may be removed if json-c fixes their inttypes header stuff in the future. Add preprocessor definitions on Windows to disable MSVC warnings about CRT secure and nonstandard functions. While there may be a better solution, this is needed to be able to see other more serious warnings. Add missing file comment block and copyright statement for clamsubmit.c. Also change json-c/json.h include filename to json.h in clamsubmit.c. The directory name is not required. Changed the hash table data integer type from long, which is poorly defined, to size_t -- which is capable of storing a pointer. Fixed a bunch of casts regarding this variable to eliminate warnings. Fixed two bugs causing utf8 encoding unit tests to fail on Windows: - The in_size variable should be the number of bytes, not the character count. This was was causing the SHIFT_JIS (japanese codepage) to UTF8 transcoding test to only transcode half the bytes. - It turns out that the MultiByteToWideChar() API can't transcode UTF16-BE to UTF16-LE. The solution is to just iterate over the buffer and flip the bytes on each uint16_t. This but was causing the UTF16-BE to UTF8 tests to fail. I also split up the utf8 transcoding tests into separate tests so I could see all of the failures instead of just the first one. Added a flags parameter to the unit test function to open testfiles because it turns out that on Windows if a file contains the \r\n it will replace it with just \n if you opened the file as a text file instead of as binary. However, if we open the CBC files as binary, then a bunch of bytecode tests fail. So I've changed the tests to open the CBC files in the bytecode tests as text files and open all other files as binary. Ported the feature tests from shell scripts to Python using a modified version of our QA test-framework, which is largely compatible and will allow us to migrate some QA tests into this repo. I'd like to add GitHub Actions pipelines in the future so that all public PR's get some testing before anyone has to manually review them. The clamd --log option was missing from the help string, though it definitely works. I've added it in this commit. It appears that clamd.c was never clang-format'd, so this commit also reformats clamd.c. Some of the check_clamd tests expected the path returned by clamd to match character for character with original path sent to clamd. However, as we now evaluate real paths before a scan, the path returned by clamd isn't going to match the relative (and possibly symlink-ridden) path passed to clamdscan. I fixed this test by changing the test to search for the basename: <signature> FOUND within the response instead of matching the exact path. Autotools: Link check_clamd with libclamav so we can use our utility functions in check_clamd.c.
Enabled the metadata collection feature, scan heuristics, and all-match mode when fuzzing in the interest of better code coverage. Also remove deprecated STREAM command.
Also creates a ZIP for non-Admin (per-user) installs. WIX requires the license file to have a .txt or .rtf extension so I added the .txt extension. I've taken the opportunity to migrate the 3rd party licenses to a COPYING subdirectory and have added licensing details to the README.md file. To build the installer, install WIX and simply run `cpack -C Release` Also removed the explicit --config option from the clamav-clamonacc.service file because it should not be required and isn't being generated correctly when using autotools anyways, especially after changes in this commit.
Visual Studio projects removed in favor of CMake because it's far easier to build and maintain. Also removed the old InnoSetup installer now that CMake's CPack provides installer creation. While working on this I found that the THIS_IS_CLAMAV macro was missing, resulting in warnings for the `have_rar` and `have_clamjit` exported global variables. I also stumbled across some code duplication and more cl_error_t / int type issues in the pcre code, so this commit includes a little cleanup.
Updates to fix issues in the CMake install instructions. Updates the README.md to indicate that CMake is now preferred Adds a GitHub Actions badge, Discord badge, and logo to the README.md. CMake: - Renamed ENABLE_DOCS to ENABLE_MAN_PAGES. - Fixed build issue when milter isn't enabled on Linux. Changed the default to build milter on non-macOS, non-Windows operating systems. - Fix LD_LIBRARY_PATH for tests including on macOS where LD_LIBRARY_PATH and DYLD_LIBRARY_PATH must be manually propagated to subprocesses. - Use UNKNOWN IMPORTED library instead of INTERFACE IMPORTED library for pdcurses, but still use INTERFACE IMPORTED for ncurses. UNKNOWN IMPORTED appears to be required so that we can use $<TARGET_FILE_DIR:Curses::curses> to collected the pdcurses library at install time on Windows. - When building with vcpkg on Windows, CMake will automatically install your app local dependencies (aka the DLL runtime dependencies). Meanwhile, file(GET_RUNTIME_DEPENDENCIES ...) doesn't appear to work correctly with vcpkg packages. The solution is to use a custom target that has CMake perform a local install to the unit_tests directory when using vcpkg. This is in fact far easier than using GET_RUNTIME_DEPENDENCIES in the unit_tests for assembling the test environment but we can't use this method for the non-vcpkg install because it won't collect checkDynamic.dll for us because we don't install our tests. We also can't link with the static check.lib because the static check.lib has pthreads symbols linked in and will conflict with our pthread.dll. TL;DR: We'll continue to use file(GET_RUNTIME_DEPENDENCIES ...) for assembling the test enviornment on non-vcpkg builds, and use the local install method for vcpkg builds. testcase.py: Wrapped a Pathlib.unlink() call in exception handling as the missing_ok optional parameter requires a Python version too new for common use. Remove localtime_r from win32 compat lib. localtime_r may be present in libcheck when building with vcpkg and while making it a static function would also solve the issue, using localtime_s instead like we do everywhere else should work just fine. check_clamd: Limited the max # of connections for the stress test on Mac to 850, to address issues found testing on macos-latest on GitHub Actions.
Add missing CTest files to tarball. Remove the generated version.h from libclamav sources so it isn't added to the dist. version.h should be generated at build time by both autotools builds and cmake builds. When included with the dist, it may cause clamd VERSION command checks to fail because clamd is compiled with the wrong version.h header. Also bumped the minimum CMake version for Windows to accomodate the file(GET_RUNTIME_DEPENDENCIES).
On Windows, files open()'ed without the O_BINARY flag will have new-line LF (aka \n) converted to CRLF (aka \r\n) automatically when read from or written to. This is undesirable for all scan targets AND temp files because it affects pattern matching and with hashing. This commit converts a handful of instances throughout the codebase where it appears that O_BINARY was mistakenly omitted and could result in unexpected behavior on Windows. Git on Windows also converts LF -> CRLF for "text" files, for editing purposes. This is problematic for scan files and test files that should match verbatim. We can prevent this issue by marking .ref test files as "binary" in the .gitattributes file and by always opening scan files and temp files as binary. In this commit I've also removed the `ChangeLog merge=cl-merge` line that was once used to reduce ChangeLog merge conflicts by using the gnulib git-merge-changlog tool. This project now categorizes changes in the NEWS.md. For finer detail, git commit history is fully accessible on github.com.
And include the user manual, if present.
Python 3.6 is not available on Debian 9 and other older LTS releases. This patch removes use of Python f-strings which were introduced in Python 3.6 so as to support Python 3.5. TODO: Revert this commit when Debian 9 dies or gets f-string support (whichver comes first).
The test previously tried to limit the # of connections to `ulimit -n` - 5. On most linux docker containers this failed with a test timeout at about connection 285 or so. The output in test-clamd.log would look something like this: check_clamd.c:74:E:clamd stress test:test_connections:0: (after this point) Test timeout expired The same issue was observed with FreeBSD (12.2) when limiting to around 280 (noting that the FD # in the debug log actually hit around 288). Limiting the # of connections to 250 resolves the issue in our test pipeline.
The clamd socket path was changed be an absolute path when adding CTest support. This quietly broke the check_clamd libcheck program when building with autotools because a relative path was expected. I failed to notice because the autotools `make check` doesn't actually care if check_clamd works! It turns out that a relative path is required because the max length for a socket path is *very* short. This commit changes check_clamd and the associated CMake test to also use a relative path for the clamd socket. Notably it also modifies the testcase.py framework switch to the cls.path_tmp (generated) directory before the tests and restore the CWD after the tests so as to ensure that the socket file is dropped in somewhere in that tmp directory.
This commit fixes CAB & CHM unit tests on 32bit linux systems. The _LARGEFILE_SOURCE, _LARGEFILE64_SOURCE, and _FILE_OFFSET_BITS=64 variables should not be hardcoded, but should get their values from CheckFileOffsetBits.cmake, which is invoked in the top-level CMakeLists.txt. Since the current libmspack CMake support is piggybacking on ClamAV's CMakeLists.txt, it should work fine to remove the hardcoded defines. When libmspack gets its own CMake build system, it will have it's own call to include(CheckFileOffsetBits) which will take care of it.
It turns out we don't actually run the test_connections clamd test for BSD variants because they fail to connect after a couple hundred connections regardless of the fd limit and hang. This issue drew attention to the fact that our CMake tooling wasn't defining C_BSD.
In testing on Alpine, I found that most libs were installing to
<prefix>/lib while libclamav installed to <prefix>/lib64. Those who like
multiarch will advocate for lib64, though I only actually noticed it
because clamscan failed to find libclamav.so! Anyways, they should all
install to lib64 by default if that's what how the system is set up.
Using ${CMAKE_INSTALL_FULL_LIBDIR} instead of <prefix>/lib will do that.
Adds support to the pcre2 and pthreadw32 Find<Package>.cmake modules for correctly discovering the debug versions. This change modeled after the upstream FindBZip2.cmake module. Also eliminated HAVE_STRUCT_TIMESPEC redefinition warnings in Windows builds.
And the Visual Studio solution has been removed.
micahsnyder
pushed a commit
that referenced
this pull request
Jun 24, 2021
…options Added windows service command line options in clamd and freshclam
micahsnyder
added a commit
that referenced
this pull request
May 30, 2023
If a signature has a pattern that is too short will fail to load the siganture but does not cause the entire load process to abort. This is bad for two reasons: 1) It is not immediately apparent that the signature is bad, and so it could be published accidentally. 2) The signature is partially loaded by the time the bad pattern is observed and that may cause a crash later. Because of #1, it is not worth it to try to unload the first part of the signature. Instead, we should just abort the signature load. Fixes: Cisco-Talos#923
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test PR for actions