Skip to content

Commit

Permalink
Fix several coverity issues in UnRAR interface code
Browse files Browse the repository at this point in the history
Fix Coverity issues 192935, 192932, 192928, and 192917.

None of these are particularly serious. I thought I'd clean them up
while trying to track down a strange crash that occurs in Windows debug
builds with my specific setup when freeing the metadata filename pointer
malloced by the UnRAR iface "peek" function.

I wasn't able to figure out why freeing that causes a crash, so instead
I converted it to an array that need not be freed, and my troubles
melted away.
  • Loading branch information
micahsnyder committed Dec 6, 2023
1 parent d1252ba commit f64a594
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 24 deletions.
9 changes: 0 additions & 9 deletions libclamav/scanners.c
Original file line number Diff line number Diff line change
Expand Up @@ -473,10 +473,6 @@ static cl_error_t cli_scanrar_file(const char *filepath, int desc, cli_ctx *ctx)
/*
* Free up any malloced metadata...
*/
if (metadata.filename != NULL) {
free(metadata.filename);
metadata.filename = NULL;
}
if (NULL != filename_base) {
free(filename_base);
filename_base = NULL;
Expand Down Expand Up @@ -512,11 +508,6 @@ static cl_error_t cli_scanrar_file(const char *filepath, int desc, cli_ctx *ctx)
filename_base = NULL;
}

if (metadata.filename != NULL) {
free(metadata.filename);
metadata.filename = NULL;
}

if (NULL != extract_fullpath) {
free(extract_fullpath);
extract_fullpath = NULL;
Expand Down
30 changes: 16 additions & 14 deletions libclamunrar_iface/unrar_iface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ cl_unrar_error_t unrar_open(const char* filename, void** hArchive, char** commen
if (archiveData == NULL) {
unrar_dbgmsg("unrar_open: Not enough memory to allocate main archive header data structure.\n");
status = UNRAR_EMEM;
goto done;
}
archiveData->ArcName = (char*)filename;
archiveData->OpenMode = RAR_OM_EXTRACT;
Expand All @@ -220,6 +221,7 @@ cl_unrar_error_t unrar_open(const char* filename, void** hArchive, char** commen
if (archiveData->CmtBuf == NULL) {
unrar_dbgmsg("unrar_open: Not enough memory to allocate main archive header comment buffer.\n");
status = UNRAR_EMEM;
goto done;
}
archiveData->CmtBufSize = CMTBUFSIZE;

Expand All @@ -231,23 +233,24 @@ cl_unrar_error_t unrar_open(const char* filename, void** hArchive, char** commen
}

switch (archiveData->CmtState) {
case 0: {
case ERAR_SUCCESS: {
unrar_dbgmsg("unrar_open: Comments are not present in this archive.\n");
break;
}
case ERAR_BAD_DATA: {
unrar_dbgmsg("unrar_open: Archive Comments may be broken.\n");
break;
}
case ERAR_SMALL_BUF: {
unrar_dbgmsg("unrar_open: Archive Comments are not present in this file.\n");
unrar_dbgmsg("unrar_open: Comment buffer was too small, comments are not read completely.\n");
break;
}
case 1: {
unrar_dbgmsg("unrar_open: Archive Comments:\n\t %s\n", archiveData->CmtBuf);
unrar_dbgmsg("unrar_open: Archive Comments read completely.\n");
break;
}
case ERAR_NO_MEMORY: {
unrar_dbgmsg("unrar_open: Memory error when reading archive comments!\n");
status = UNRAR_EMEM;
unrar_dbgmsg("unrar_open: Not enough memory to extract comments!\n");
break;
}
default: {
Expand All @@ -261,7 +264,6 @@ cl_unrar_error_t unrar_open(const char* filename, void** hArchive, char** commen
if (NULL == *comment) {
unrar_dbgmsg("unrar_open: Error duplicating comment buffer.\n");
*comment_size = 0;
status = UNRAR_EMEM;
}
}

Expand Down Expand Up @@ -306,7 +308,7 @@ cl_unrar_error_t unrar_peek_file_header(void* hArchive, unrar_metadata_t* file_m
struct RARHeaderDataEx headerData;
int read_header_ret = 0;

wchar_t RedirName[1024];
wchar_t RedirName[2048];

memset(&headerData, 0, sizeof(struct RARHeaderDataEx));

Expand All @@ -324,9 +326,9 @@ cl_unrar_error_t unrar_peek_file_header(void* hArchive, unrar_metadata_t* file_m
headerData.CmtBuf = NULL;
headerData.CmtBufSize = 0;

headerData.RedirNameSize = 1024 * sizeof(wchar_t);
headerData.RedirNameSize = 2048;
headerData.RedirName = (wchar_t*)&RedirName;
memset(headerData.RedirName, 0, headerData.RedirNameSize);
memset(headerData.RedirName, 0, headerData.RedirNameSize * sizeof(wchar_t));

read_header_ret = RARReadHeaderEx(hArchive, &headerData);
if (ERAR_SUCCESS != read_header_ret) {
Expand All @@ -336,11 +338,11 @@ cl_unrar_error_t unrar_peek_file_header(void* hArchive, unrar_metadata_t* file_m

file_metadata->unpack_size = headerData.UnpSize + ((int64_t)headerData.UnpSizeHigh << 32);
file_metadata->pack_size = headerData.PackSize + ((int64_t)headerData.PackSizeHigh << 32);
file_metadata->filename = unrar_strndup(headerData.FileName, 1024);
file_metadata->crc = headerData.FileCRC;
file_metadata->encrypted = (headerData.Flags & RHDF_ENCRYPTED) ? 1 : 0;
file_metadata->is_dir = (headerData.Flags & RHDF_DIRECTORY) ? 1 : 0;
file_metadata->method = headerData.Method;
strncpy(file_metadata->filename, headerData.FileName, 1024);
file_metadata->crc = headerData.FileCRC;
file_metadata->encrypted = (headerData.Flags & RHDF_ENCRYPTED) ? 1 : 0;
file_metadata->is_dir = (headerData.Flags & RHDF_DIRECTORY) ? 1 : 0;
file_metadata->method = headerData.Method;

unrar_dbgmsg("unrar_peek_file_header: Name: %s\n", headerData.FileName);
unrar_dbgmsg("unrar_peek_file_header: Directory?: %u\n", file_metadata->is_dir);
Expand Down
2 changes: 1 addition & 1 deletion libclamunrar_iface/unrar_iface.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ typedef enum cl_unrar_error_tag {
typedef struct unrar_metadata_tag {
uint64_t pack_size;
uint64_t unpack_size;
char *filename;
char filename[1024];
uint32_t crc;
unsigned int encrypted;
uint8_t method;
Expand Down

0 comments on commit f64a594

Please sign in to comment.