Skip to content

Fix minor memory leak#1489

Merged
val-ms merged 4 commits into
Cisco-Talos:mainfrom
val-ms:oss-fuzz-engine-free-certs_directory
May 7, 2025
Merged

Fix minor memory leak#1489
val-ms merged 4 commits into
Cisco-Talos:mainfrom
val-ms:oss-fuzz-engine-free-certs_directory

Conversation

@val-ms
Copy link
Copy Markdown
Contributor

@val-ms val-ms commented Apr 21, 2025

The memory allocated for certs_directory in the ClamAV engine is not free'd when the engine is free'd. This isn't readily apparent when using a mempool because the mempool itself is free'd and the issue is masked.

Also: I found that the location in cl_engine_free() where we estimate the number of tasks incorrectly placed the fuzzy_hashmap free task in the block where we free the test_root, rather than up in the engine->root for-loop. Fixed.

Edit: I have also added a fix for a leak when multiple idb signatures are loaded. I discovered this while trying to come up with simple repro steps for anyone wishing to manually test this fix.

Repro steps on Linux, not Mac (requires valgrind):

  1. Build clamav with the cmake option -D DISABLE_MPOOL=ON

  2. Run ctest -V

The memory allocated for certs_directory in the ClamAV engine is not
free'd when the engine is free'd. This isn't readily apparent when using
a mempool because the mempool itself is free'd and the issue is masked.

Also: I found that the location in `cl_engine_free()` where we estimate
the number of tasks incorrectly placed the fuzzy_hashmap free task in
the block where we free the `test_root`, rather than up in the
`engine->root` for-loop. Fixed.
@val-ms val-ms force-pushed the oss-fuzz-engine-free-certs_directory branch from 597a7e4 to f3c2a02 Compare April 28, 2025 15:51
Comment thread libclamav/others.h
struct icon_matcher {
char **group_names[2];
unsigned int group_counts[2];
uint32_t group_counts[2];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious as to why the change from unsigned int, which is almost always 32-bits, to uint32_t was made. Should I make similar changes when I edit older code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lengths of older C types, such as char, short, int, long, vary by OS and arch. int is more reliably 32-bits than long, but both may vary by OS and architecture: https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models

I habitually switch unsigned, int, and long types to stdint.h types like uint32_t and uint64_t. For data, uint8_t is better than unsigned char. For strings, char is fine. int8_t might technically be better to represent data underlying utf8. utf8 is hard in C, and ClamAV code generally does a bad job with it.

Weirdly enough, off_t is also poorly defined. It's still everywhere and ought to be replaced with ssize_t if negative values need to be supported, or size_t otherwise. Most clamav offsets are relative to the start of the file so negative is almost never required. Disclaimer: I've introduced bugs by converting types like this from signed to unsigned, so you have to be really careful when doing that.

On that topic, sentinel values for indicating an error are a code smell. Ideally we'd never use them and would always return an error code (e.g. using cl_error_t) and then pass the back values through function parameters. It's just considerable effort to rewrite to do it that way with existing code, and there is risk we'd introduce a bug by switching.

Comment thread libclamav/readdb.c Outdated
jhumlick
jhumlick previously approved these changes Apr 29, 2025
@val-ms val-ms force-pushed the oss-fuzz-engine-free-certs_directory branch from f3c2a02 to 8cbe5af Compare April 30, 2025 18:00
jhumlick
jhumlick previously approved these changes Apr 30, 2025
val-ms added 2 commits May 1, 2025 13:58
The logic for loading an icon matcher assumes that only one .idb file is
loaded. If a second is loaded, the first is forgotten (memory leak).

This commit checks to see if `engine->iconcheck` is already allocated
and if so it will use that instead of allocating a new one.

I also cleaned up the error handling in this function, using goto-done
error handling.

I added proper cleanup for freeing the matcher in case of an idb
signature load error, copied from `cl_engine_free()`.
Notably: resolve atty and openssl security warnings.
@val-ms val-ms force-pushed the oss-fuzz-engine-free-certs_directory branch from 8cbe5af to 6f0520b Compare May 1, 2025 19:08
jhumlick
jhumlick previously approved these changes May 2, 2025
The .sign test files have the min flevel set to 220.
It should be 230.

Also upgrade clamav-signature-util to v1.2.4 for fix so new .sign
files will have the correct min flevel.
@val-ms val-ms force-pushed the oss-fuzz-engine-free-certs_directory branch from 6f0520b to 544fb9f Compare May 5, 2025 20:54
@val-ms val-ms merged commit 66c049f into Cisco-Talos:main May 7, 2025
22 of 24 checks passed
@val-ms val-ms deleted the oss-fuzz-engine-free-certs_directory branch May 7, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants