Skip to content

Clam 2296 coverity regressions #891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
val-ms merged 11 commits into from
Apr 13, 2023
Merged

Clam 2296 coverity regressions #891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c7f529f
Coverity-405735: Add missing 'goto done;' on error
val-ms Apr 7, 2023
659eced
Coverity-405734: Add missing variable initializer
val-ms Apr 7, 2023
2e69cde
Coverity-405733, 405732: Add missing variable initializers
val-ms Apr 7, 2023
af27c0a
Coverity-405726, 405725: Fix overlapping copy complaint
val-ms Apr 7, 2023
865a93a
Coverity-404677: Add missing frees in sigtool
val-ms Apr 7, 2023
30cada2
Coverity-401434: Switch to realloc that does not free after failure
val-ms Apr 7, 2023
0246386
Coverity-401433: Fix file descriptor leak in VBA parser
val-ms Apr 7, 2023
d04daab
Fix possibly uninitialized variable warning
val-ms Apr 7, 2023
526c6b6
Fix benign compiler warning in logging macro
val-ms Apr 7, 2023
d968cd2
Fix many warnings
val-ms Apr 8, 2023
c8fcabe
Coverity-405915, 405916: False positives
val-ms Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clamav-milter/clamav-milter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ int main(int argc, char **argv)
char myname[255];

if (((opt = optget(opts, "ReportHostname"))->enabled &&
strncpy(myname, opt->strarg, sizeof(myname))) ||
!gethostname(myname, sizeof(myname))) {
strncpy(myname, opt->strarg, sizeof(myname) - 1)) ||
!gethostname(myname, sizeof(myname) - 1)) {

myname[sizeof(myname) - 1] = '\0';
snprintf(xvirushdr, sizeof(xvirushdr), "clamav-milter %s at %s",
Expand Down
2 changes: 1 addition & 1 deletion clamav-milter/clamfi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ static sfsistat (*InfectedAction)(SMFICTX *ctx);
static char *rejectfmt = NULL;

int addxvirus = 0; /* 0 - don't add | 1 - replace | 2 - add */
char xvirushdr[255];
char xvirushdr[300];
char *viraction = NULL;
int multircpt = 1;

Expand Down
2 changes: 1 addition & 1 deletion clamav-milter/clamfi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

extern uint64_t maxfilesize;
extern int addxvirus;
extern char xvirushdr[255];
extern char xvirushdr[300];
extern int multircpt;

sfsistat clamfi_body(SMFICTX *ctx, unsigned char *bodyp, size_t len);
Expand Down
1 change: 0 additions & 1 deletion clamonacc/client/client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,6 @@ cl_error_t onas_setup_client(struct onas_context **ctx)
{

const struct optstruct *opts;
const struct optstruct *opt;
cl_error_t err;
int remote;

Expand Down
10 changes: 0 additions & 10 deletions examples/ex_cl_cvdunpack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,12 @@
*/
int main(int argc, char **argv)
{
int fd;
cl_error_t ret;

const char *filename;
const char *destination_directory;
bool dont_verify = false;

char dest_buff[1024];

unsigned long int size = 0;
unsigned int sigs = 0;
long double mb;
const char *virname;
struct cl_engine *engine;
struct cl_scan_options options;

switch (argc) {
case 2:
filename = argv[1];
Expand Down
5 changes: 4 additions & 1 deletion examples/ex_file_inspection_callback.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ cl_error_t post_callback(
const char *virname,
void *context) // Could be used to retrieve / store contextual information for app
{
(void)fd;
(void)context;

printf("result: %d\n", result);
printf("virname: %s\n", virname);
printf("\n\n");
Expand All @@ -120,7 +123,7 @@ int main(int argc, char **argv)
long double mb;
const char *virname;
const char *filename;
struct cl_engine *engine;
struct cl_engine *engine = NULL;
struct cl_scan_options options;
char database_filepath[256];
bool created_database = false;
Expand Down
2 changes: 1 addition & 1 deletion examples/ex_prescan_callback.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ int main(int argc, char **argv)
long double mb;
const char *virname;
const char *filename;
struct cl_engine *engine;
struct cl_engine *engine = NULL;
struct cl_scan_options options;

if (argc != 2) {
Expand Down
2 changes: 1 addition & 1 deletion libclamav/bytecode_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2315,7 +2315,7 @@ int32_t cli_bcapi_json_get_string(struct cli_bc_ctx *ctx, int8_t *str, int32_t s
return str_len;
} else {
/* limit on len+1 */
strncpy((char *)str, jstr, len);
memcpy((char *)str, jstr, len);
str[len] = '\0';
return len + 1;
}
Expand Down
38 changes: 33 additions & 5 deletions libclamav/bytecode_vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,8 @@ static always_inline struct stack_entry *pop_stack(struct stack *stack,
READ1(op1, BINOP(1)); \
sop0 = op0; \
sop1 = op1; \
(void)sop0; \
(void)sop1; \
OP; \
W0(inst->dest, res); \
break; \
Expand All @@ -399,6 +401,8 @@ static always_inline struct stack_entry *pop_stack(struct stack *stack,
READ8(op1, BINOP(1)); \
sop0 = op0; \
sop1 = op1; \
(void)sop0; \
(void)sop1; \
OP; \
W1(inst->dest, res); \
break; \
Expand All @@ -410,6 +414,8 @@ static always_inline struct stack_entry *pop_stack(struct stack *stack,
READ16(op1, BINOP(1)); \
sop0 = op0; \
sop1 = op1; \
(void)sop0; \
(void)sop1; \
OP; \
W2(inst->dest, res); \
break; \
Expand All @@ -421,6 +427,8 @@ static always_inline struct stack_entry *pop_stack(struct stack *stack,
READ32(op1, BINOP(1)); \
sop0 = op0; \
sop1 = op1; \
(void)sop0; \
(void)sop1; \
OP; \
W3(inst->dest, res); \
break; \
Expand All @@ -432,6 +440,8 @@ static always_inline struct stack_entry *pop_stack(struct stack *stack,
READ64(op1, BINOP(1)); \
sop0 = op0; \
sop1 = op1; \
(void)sop0; \
(void)sop1; \
OP; \
W4(inst->dest, res); \
break; \
Expand Down Expand Up @@ -534,6 +544,24 @@ static always_inline struct stack_entry *pop_stack(struct stack *stack,
break; \
}

#define DEFINE_OP_BC_RET_VOID(OP, T) \
case OP: { \
operand_t ret; \
CHECK_GT(stack_depth, 0); \
stack_depth--; \
stack_entry = pop_stack(&stack, stack_entry, &func, &ret, &bb, \
&bb_inst); \
values = stack_entry ? stack_entry->values : ctx->values; \
CHECK_GT(func->numBytes, ret); \
if (!bb) { \
stop = CL_BREAK; \
continue; \
} \
stackid = ptr_register_stack(&ptrinfos, values, 0, func->numBytes) >> 32; \
inst = &bb->insts[bb_inst]; \
break; \
}

struct ptr_info {
uint8_t *base;
uint32_t size;
Expand Down Expand Up @@ -797,11 +825,11 @@ cl_error_t cli_vm_execute(const struct cli_bc *bc, struct cli_bc_ctx *ctx, const
DEFINE_OP_BC_RET_N(OP_BC_RET * 5 + 3, uint32_t, READ32, WRITE32);
DEFINE_OP_BC_RET_N(OP_BC_RET * 5 + 4, uint64_t, READ64, WRITE64);

DEFINE_OP_BC_RET_N(OP_BC_RET_VOID * 5, uint8_t, (void), (void));
DEFINE_OP_BC_RET_N(OP_BC_RET_VOID * 5 + 1, uint8_t, (void), (void));
DEFINE_OP_BC_RET_N(OP_BC_RET_VOID * 5 + 2, uint8_t, (void), (void));
DEFINE_OP_BC_RET_N(OP_BC_RET_VOID * 5 + 3, uint8_t, (void), (void));
DEFINE_OP_BC_RET_N(OP_BC_RET_VOID * 5 + 4, uint8_t, (void), (void));
DEFINE_OP_BC_RET_VOID(OP_BC_RET_VOID * 5, uint8_t);
DEFINE_OP_BC_RET_VOID(OP_BC_RET_VOID * 5 + 1, uint8_t);
DEFINE_OP_BC_RET_VOID(OP_BC_RET_VOID * 5 + 2, uint8_t);
DEFINE_OP_BC_RET_VOID(OP_BC_RET_VOID * 5 + 3, uint8_t);
DEFINE_OP_BC_RET_VOID(OP_BC_RET_VOID * 5 + 4, uint8_t);

DEFINE_ICMPOP(OP_BC_ICMP_EQ, res = (op0 == op1));
DEFINE_ICMPOP(OP_BC_ICMP_NE, res = (op0 != op1));
Expand Down
6 changes: 3 additions & 3 deletions libclamav/crtmgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,11 +394,11 @@ static cl_error_t crtmgr_get_recov_data(BIGNUM *sig, cli_crt *x509,
uint8_t **buffer, uint8_t **payload,
int *payload_len)
{
BN_CTX *bnctx;
BN_CTX *bnctx = NULL;
int pad_size;
int keylen;
uint8_t *d;
BIGNUM *x;
uint8_t *d = NULL;
BIGNUM *x = NULL;
cl_error_t ret;

*buffer = NULL;
Expand Down
8 changes: 8 additions & 0 deletions libclamav/disasm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,9 +1316,11 @@ static const uint8_t *disasm_x86(const uint8_t *command, unsigned int len, struc
switch (x87_st[table][rm].args) {
case X87_S:
reversed = 1;
/* fall-through */
case X87_R:
s->args[reversed ^ 1].access = ACCESS_REG;
s->args[reversed ^ 1].reg = X86_REG_ST0;
/* fall-through */
case X87_ONE:
s->args[reversed].access = ACCESS_REG;
s->args[reversed].reg = X86_REG_ST0 + (rm & 7);
Expand Down Expand Up @@ -1403,11 +1405,13 @@ static const uint8_t *disasm_x86(const uint8_t *command, unsigned int len, struc
case ADDR_REG_SS:
case ADDR_REG_DS:
assert(x86ops[table][s->table_op].dsize == SIZE_WORD);
/* fall-through */
case ADDR_REG_ESP:
case ADDR_REG_EBP:
case ADDR_REG_ESI:
case ADDR_REG_EDI:
assert(x86ops[table][s->table_op].dsize != SIZE_BYTE && x86ops[table][s->table_op].dsize != SIZE_BYTEH);
/* fall-through */
case ADDR_REG_EAX:
case ADDR_REG_ECX:
case ADDR_REG_EDX:
Expand Down Expand Up @@ -1453,6 +1457,7 @@ static const uint8_t *disasm_x86(const uint8_t *command, unsigned int len, struc
case ADDR_MRM_GEN_CR:
case ADDR_MRM_GEN_DR:
reversed = 1;
/* fall-through */

case ADDR_MRM_GEN_EG:
case ADDR_MRM_GEN_ES:
Expand Down Expand Up @@ -1500,8 +1505,10 @@ static const uint8_t *disasm_x86(const uint8_t *command, unsigned int len, struc
break;
case SIZE_WD:
s->args[reversed].size += (s->opsize == 0);
/* fall-through */
case SIZE_WORD:
s->args[reversed].size++;
/* fall-through */
case SIZE_BYTE:
break;
default:
Expand Down Expand Up @@ -1743,6 +1750,7 @@ const uint8_t *cli_disasm_one(const uint8_t *buff, unsigned int len,
break;
case ACCESS_REG:
w->arg[i][1] = s.args[i].reg;
/* fall-through */
default:
cli_writeint32(&w->arg[i][2], s.args[i].arg.q);
cli_writeint32(&w->arg[i][6], s.args[i].arg.q >> 32);
Expand Down
9 changes: 6 additions & 3 deletions libclamav/dsig.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ static unsigned char *cli_decodesig(const char *sig, unsigned int plen, BIGNUM *
int i, slen = strlen(sig), dec;
unsigned char *plain = NULL, *ret_sig = NULL;
BIGNUM *r = NULL, *p = NULL, *c = NULL;
BN_CTX *bn_ctx;
BN_CTX *bn_ctx = NULL;
unsigned int bn_bytes;
;

Expand Down Expand Up @@ -331,7 +331,8 @@ cl_error_t cli_versig(const char *md5, const char *dsig)
#define BLK_LEN (PAD_LEN - HASH_LEN - 1)
int cli_versig2(const unsigned char *sha256, const char *dsig_str, const char *n_str, const char *e_str)
{
unsigned char *decoded, digest1[HASH_LEN], digest2[HASH_LEN], digest3[HASH_LEN], *salt;
unsigned char *decoded = NULL;
unsigned char digest1[HASH_LEN], digest2[HASH_LEN], digest3[HASH_LEN], *salt;
unsigned char mask[BLK_LEN], data[BLK_LEN], final[8 + 2 * HASH_LEN], c[4];
unsigned int i, rounds;
void *ctx;
Expand Down Expand Up @@ -360,8 +361,8 @@ int cli_versig2(const unsigned char *sha256, const char *dsig_str, const char *n
}

if (decoded[PAD_LEN - 1] != 0xbc) {
free(decoded);
ret = CL_EVERIFY;
goto done;
}
BN_free(n);
BN_free(e);
Expand All @@ -372,6 +373,7 @@ int cli_versig2(const unsigned char *sha256, const char *dsig_str, const char *n
memcpy(mask, decoded, BLK_LEN);
memcpy(digest2, &decoded[BLK_LEN], HASH_LEN);
free(decoded);
decoded = NULL;

c[0] = c[1] = 0;
rounds = (BLK_LEN + HASH_LEN - 1) / HASH_LEN;
Expand Down Expand Up @@ -417,6 +419,7 @@ int cli_versig2(const unsigned char *sha256, const char *dsig_str, const char *n
return memcmp(digest1, digest2, HASH_LEN) ? CL_EVERIFY : CL_SUCCESS;

done:
free(decoded);
Copy link
Contributor

@ragusaa ragusaa Apr 10, 2023

Choose a reason for hiding this comment

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

It is specified in the man page to not do anything if 'decoded' is NULL. Are there any platforms that do not honor this? Should we add a test just to be safe?

Copy link
Contributor Author

@val-ms val-ms Apr 11, 2023

Choose a reason for hiding this comment

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

I used to be superstitious about this, but from what I can tell it's safe everywhere if the pointer is NULL.
E.g. on Windows: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/free?view=msvc-170

BN_free(n);
BN_free(e);
return ret;
Expand Down
25 changes: 16 additions & 9 deletions libclamav/explode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ int explode_init(struct xplstate *X, uint16_t flags)
} \
}

#define GETCODES(CASE, WHICH, HOWMANY) \
case CASE: { \
#define GETCODES(WHICH, HOWMANY) \
{ \
if (!X->avail_in) return EXPLODE_EBUFF; \
if (!X->got) \
need = *X->next_in; \
Expand Down Expand Up @@ -227,6 +227,7 @@ int explode_init(struct xplstate *X, uint16_t flags)

#define SETCASE(CASE) \
X->state = (CASE); \
/* fall-through */ \
case (CASE): { /* FAKE */ \
}

Expand All @@ -236,14 +237,19 @@ int explode(struct xplstate *X)
int temp = -1;

switch (X->state) {
/* grab compressed coded literals, if present */
GETCODES(GRABLITS, lit_tree, 256);
/* grab compressed coded lens */
GETCODES(GRABLENS, len_tree, 64);
/* grab compressed coded dists */
GETCODES(GRABDISTS, dist_tree, 64);
case GRABLITS: { /* grab compressed coded literals, if present */
GETCODES(lit_tree, 256);
} /* fall-through */

case EXPLODE:
case GRABLENS: { /* grab compressed coded lens */
GETCODES(len_tree, 64);
} /* fall-through */

case GRABDISTS: { /* grab compressed coded dists */
GETCODES(dist_tree, 64);
} /* fall-through */

case EXPLODE: {
while (X->avail_in || X->bits) {
GETBIT; /* can't fail */
if (val) {
Expand Down Expand Up @@ -315,6 +321,7 @@ int explode(struct xplstate *X)
}
X->state = EXPLODE;
}
}
}
return EXPLODE_EBUFF;
}
Expand Down
4 changes: 2 additions & 2 deletions libclamav/filtering.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ int filter_add_static(struct filter *m, const unsigned char *pattern, unsigned l
for (j = 0; (best < 100 && j < MAX_CHOICES) || (j < maxlen); j++) {
uint32_t num = MAXSOPATLEN;
uint8_t k;
if (j + 2 > len)
if ((unsigned long)(j + 2) > len)
break;
for (k = j; k < len - 1 && (k - j < MAXSOPATLEN); k++) {
q = cli_readint16(&pattern[k]);
Expand Down Expand Up @@ -323,7 +323,7 @@ enum badness {
};
static inline void get_score(enum badness badness, unsigned i, const struct filter *m, const struct char_spec *spec0, const struct char_spec *spec1, int32_t *score, int32_t *score_end)
{
int32_t base;
int32_t base = 0;
unsigned k0, k1, num_introduced = 0, num_end_introduced = 0;
switch (badness) {
case reject:
Expand Down
4 changes: 2 additions & 2 deletions libclamav/hwp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1525,7 +1525,7 @@ static inline cl_error_t parsehwp3_infoblk_1(cli_ctx *ctx, fmap_t *dmap, size_t
char field[HWP3_FIELD_LENGTH];
#endif
#if HAVE_JSON
json_object *infoblk_1, *contents, *counter, *entry;
json_object *infoblk_1, *contents = NULL, *counter, *entry = NULL;
#endif

hwp3_debug("HWP3.x: Information Block @ offset %llu\n", infoloc);
Expand Down Expand Up @@ -1757,7 +1757,7 @@ static cl_error_t hwp3_cb(void *cbdata, int fd, const char *filepath, cli_ctx *c
int i, p = 0, last = 0;
uint16_t nstyles;
#if HAVE_JSON
json_object *fonts;
json_object *fonts = NULL;
#endif

UNUSEDPARAM(filepath);
Expand Down
Loading