Clam 2324 more coverity fixes 2 #899
Merged
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
micahsnyder
force-pushed
the
CLAM-2324-more-coverity-fixes-2
branch
from
89e5977
to
4946076
Compare
RTF: - Coverity-344490: Use cli_realloc instead of cli_realloc2. cli_realloc2 will free the memory if the allocation fails, though we also free the memory later in SCAN_CLEANUP. - Fix warning about unused variable. AutoIt: - Fix possible memory leaks of input and output buffers. - Set pointer to NULL after handing off memory to new pointer.
Coverity-344508: Fix out-of-bound read in check_str test. The len argument cannot be longer than the size of the source buffer. The original test was attempting to test an append failure. The updated test checks for correct behavior with two consecutive appends. Also added function comments to document correct use of textbuffer functions. Coverity-344493: Fix out-of-bounds read in check_jsnorm test. The buffers passed to tokenizer_test must be NULL-terminated.
If not initialized, it could try to close some random file descriptor.
…module Fix possibly unitialized binop variable in bytecode module for STORE and COPY instructions in bytecode module. Refactored slightly to include additional opcode login in the switch statement.
Prevent double-extraction of same PDF object Two indirect references to the same PDF object may cause it to try to extract that object twice. This also may cause it to set the extraction path twice, which leaks the memory from the first time. This commit records when object extraction is attempted and prevents doing it again. It also adds a couple extra checks to make sure that the object path string is not leaked. Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58072 Also: - Coverity-317959: Fix complaint about logically dead code. No need to check if UE variable is NULL because we would've returned earlier if it was NULL. - A bunch of medium-severity coverity issues for PDF parser regarding checking if a `pdf` pointer is NULL after dereferencing it. - Coverity-192930: bytes_remaining was being checked twice in a row without chainging it. Turns out we should have been changing it after moving the `index` pointer. - Coverity-192920: Switch to use CLI_REALLOC instead of cli_realloc2. This is because cli_realloc2 would free `pdf->objs` on failure and we still need it.
Coverity-344510: Fix unitialized sock variable in check_clamd test program. Only close the socket on error if is a valid file descriptor. Coverity-344507: Remove unused file-open from clamd test. Coverity-344497: clamd test recvpartial convenience function is was reusing the `len` variable used for "how long is the reply" also as the buffer length. Coverity appears to be confused here and thinks that the length of the buffer may not be long enough for the NULL terminating character. I have reworked this to use a separate variable for managing the length of the buffer.
Coverity is unhappy with the use of the EC32, cli_readint32, and cli_writeint32 macros (and the 64bit equivalents to potentially change the endianess of variables in place. It claims: overlapping_assignment: Assigning ... to ..., which have overlapping memory locations and different types. Using a temporary variable in between reading and writing should resolve these "high impact" complaints. Resolves: Coverity-225232. 225225, 225215, 225212, 225180, 225170, 225165, 225161, 225159.
Coverity-225186, 225156: Fix possible leak of comment message in case parsing the comment header fails after allocating the comment buffer. Coverity-225184: Fix possible leak of egg block if the archive is not solid and contains no files. Additional improvements to egg parser error handling for functions that pass back allocated memory through the parameters. Instead of checking for failure before freeing the allocated memory, we'll hand off ownership of the allocated memory to the parameter variable by setting to NULL afterwards, and then always free the variable if not NULL after the `done` label.
For some reason we're generating a filename wiith a random hash in it to use for the comment content in the event that codepage converstion to utf8 fails for the comment. This makes no sense. So I'm removing it and letting it just fail out. The calling functions ignore the failure anyways and move on which is good. Note: I think the "cli_genfname" call that I'm removing was a copypaste from the logic for converting the filename to utf8. We still do that. I'm not sure about the consequence of failing to have a filename in that case, so I'm going to leave it as-is.
micahsnyder
force-pushed
the
CLAM-2324-more-coverity-fixes-2
branch
from
4946076
to
231aeeb
Compare
|
@ragusaa Ok added the egg parser |
ragusaa
requested changes
Apr 26, 2023
|
Since there was a change in bytecode.c, I re-ran the bytecode tests with the interpreter and the LLVM runtime. There were no issues there, so I am approving. |
ragusaa
approved these changes
Apr 26, 2023
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.
Fixes a large range of warnings from Coverity, plus one new small leak detected by oss-fuzz.