From d88f0784c1400a06efb1b09d0bfcfa31c284c7d7 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Sun, 16 Oct 2016 19:28:01 +0100 Subject: [PATCH] Tidying: coverity issues --- src/src/auths/auth-spa.c | 8 + src/src/deliver.c | 25 +++- src/src/exim_dbmbuild.c | 8 +- src/src/lookups/cdb.c | 266 +++++++++++++++++++-------------- src/src/rda.c | 11 +- src/src/spool_in.c | 10 +- src/src/transports/queuefile.c | 38 ++--- 7 files changed, 216 insertions(+), 150 deletions(-) diff --git a/src/src/auths/auth-spa.c b/src/src/auths/auth-spa.c index ab01d53fa..d1df7f2cb 100644 --- a/src/src/auths/auth-spa.c +++ b/src/src/auths/auth-spa.c @@ -1262,6 +1262,9 @@ spa_bytes_add(ptr, header, b, len*2); \ unicodeToString(((char*)structPtr) + IVAL(&structPtr->header.offset,0) , SVAL(&structPtr->header.len,0)/2) #define GetString(structPtr, header) \ toString((((char *)structPtr) + IVAL(&structPtr->header.offset,0)), SVAL(&structPtr->header.len,0)) + +#ifdef notdef + #define DumpBuffer(fp, structPtr, header) \ dumpRaw(fp,(US structPtr)+IVAL(&structPtr->header.offset,0),SVAL(&structPtr->header.len,0)) @@ -1277,6 +1280,8 @@ dumpRaw (FILE * fp, uschar *buf, size_t len) fprintf (fp, "\n"); } +#endif + char * unicodeToString (char *p, size_t len) { @@ -1325,6 +1330,8 @@ toString (char *p, size_t len) return buf; } +#ifdef notdef + void dumpSmbNtlmAuthRequest (FILE * fp, SPAAuthRequest * request) { @@ -1365,6 +1372,7 @@ dumpSmbNtlmAuthResponse (FILE * fp, SPAAuthResponse * response) DumpBuffer (fp, response, sessionKey); fprintf (fp, " Flags = %08x\n", IVAL (&response->flags, 0)); } +#endif void spa_build_auth_request (SPAAuthRequest * request, char *user, char *domain) diff --git a/src/src/deliver.c b/src/src/deliver.c index eb6c70515..f596cb684 100644 --- a/src/src/deliver.c +++ b/src/src/deliver.c @@ -2402,8 +2402,7 @@ will remain. Afterwards, close the reading end. */ for (addr2 = addr; addr2; addr2 = addr2->next) { - len = read(pfd[pipe_read], &status, sizeof(int)); - if (len > 0) + if ((len = read(pfd[pipe_read], &status, sizeof(int))) > 0) { int i; uschar **sptr; @@ -2420,10 +2419,24 @@ for (addr2 = addr; addr2; addr2 = addr2->next) if (testflag(addr2, af_file)) { - int local_part_length; - len = read(pfd[pipe_read], &local_part_length, sizeof(int)); - len = read(pfd[pipe_read], big_buffer, local_part_length); - big_buffer[local_part_length] = 0; + int llen; + if ( read(pfd[pipe_read], &llen, sizeof(int)) != sizeof(int) + || llen > 64*4 /* limit from rfc 5821, times I18N factor */ + ) + { + log_write(0, LOG_MAIN|LOG_PANIC, "bad local_part length read" + " from delivery subprocess"); + break; + } + /* sanity-checked llen so disable the Coverity error */ + /* coverity[tainted_data] */ + if (read(pfd[pipe_read], big_buffer, llen) != llen) + { + log_write(0, LOG_MAIN|LOG_PANIC, "bad local_part read" + " from delivery subprocess"); + break; + } + big_buffer[llen] = 0; addr2->local_part = string_copy(big_buffer); } diff --git a/src/src/exim_dbmbuild.c b/src/src/exim_dbmbuild.c index 7babc643e..611b6be38 100644 --- a/src/src/exim_dbmbuild.c +++ b/src/src/exim_dbmbuild.c @@ -478,9 +478,11 @@ if (yield == 0 || yield == 1) else { printf("dbmbuild abandoned\n"); - #if defined(USE_DB) || defined(USE_TDB) || defined(USE_GDBM) +#if defined(USE_DB) || defined(USE_TDB) || defined(USE_GDBM) + /* We created it, so safe to delete despite the name coming from outside */ + /* coverity[tainted_data] */ Uunlink(temp_dbmname); - #else +#else if (is_db) { sprintf(CS real_dbmname, "%s.db", temp_dbmname); @@ -493,7 +495,7 @@ else sprintf(CS real_dbmname, "%s.pag", temp_dbmname); Uunlink(real_dbmname); } - #endif /* USE_DB || USE_TDB */ +#endif /* USE_DB || USE_TDB */ } return yield; diff --git a/src/src/lookups/cdb.c b/src/src/lookups/cdb.c index 4ff42ab3e..3a9078a4e 100644 --- a/src/src/lookups/cdb.c +++ b/src/src/lookups/cdb.c @@ -281,135 +281,171 @@ cdb_find(void *handle, uschar **errmsg, uint *do_cache) { - struct cdb_state * cdbp = handle; - uint32 item_key_len, - item_dat_len, - key_hash, - item_hash, - item_posn, - cur_offset, - end_offset, - hash_offset_entry, - hash_offset, - hash_offlen, - hash_slotnm; - int loop; - - /* Keep picky compilers happy */ - do_cache = do_cache; - - key_hash = cdb_hash(keystring, key_len); - - hash_offset_entry = CDB_HASH_ENTRY * (key_hash & CDB_HASH_MASK); - hash_offset = cdb_unpack(cdbp->cdb_offsets + hash_offset_entry); - hash_offlen = cdb_unpack(cdbp->cdb_offsets + hash_offset_entry + 4); - - /* If the offset length is zero this key cannot be in the file */ - if (hash_offlen == 0) { - return FAIL; - } - hash_slotnm = (key_hash >> 8) % hash_offlen; - - /* check to ensure that the file is not corrupt - * if the hash_offset + (hash_offlen * CDB_HASH_ENTRY) is longer - * than the file, then we have problems.... */ - if ((hash_offset + (hash_offlen * CDB_HASH_ENTRY)) > cdbp->filelen) { - *errmsg = string_sprintf("cdb: corrupt cdb file %s (too short)", - filename); - DEBUG(D_lookup) debug_printf("%s\n", *errmsg); - return DEFER; +struct cdb_state * cdbp = handle; +uint32 item_key_len, +item_dat_len, +key_hash, +item_hash, +item_posn, +cur_offset, +end_offset, +hash_offset_entry, +hash_offset, +hash_offlen, +hash_slotnm; +int loop; + +/* Keep picky compilers happy */ +do_cache = do_cache; + +key_hash = cdb_hash(keystring, key_len); + +hash_offset_entry = CDB_HASH_ENTRY * (key_hash & CDB_HASH_MASK); +hash_offset = cdb_unpack(cdbp->cdb_offsets + hash_offset_entry); +hash_offlen = cdb_unpack(cdbp->cdb_offsets + hash_offset_entry + 4); + +/* If the offset length is zero this key cannot be in the file */ + +if (hash_offlen == 0) + return FAIL; + +hash_slotnm = (key_hash >> 8) % hash_offlen; + +/* check to ensure that the file is not corrupt + * if the hash_offset + (hash_offlen * CDB_HASH_ENTRY) is longer + * than the file, then we have problems.... */ + +if ((hash_offset + (hash_offlen * CDB_HASH_ENTRY)) > cdbp->filelen) + { + *errmsg = string_sprintf("cdb: corrupt cdb file %s (too short)", + filename); + DEBUG(D_lookup) debug_printf("%s\n", *errmsg); + return DEFER; } - cur_offset = hash_offset + (hash_slotnm * CDB_HASH_ENTRY); - end_offset = hash_offset + (hash_offlen * CDB_HASH_ENTRY); - /* if we are allowed to we use mmap here.... */ +cur_offset = hash_offset + (hash_slotnm * CDB_HASH_ENTRY); +end_offset = hash_offset + (hash_offlen * CDB_HASH_ENTRY); + +/* if we are allowed to we use mmap here.... */ + #ifdef HAVE_MMAP - /* make sure the mmap was OK */ - if (cdbp->cdb_map != NULL) { - uschar * cur_pos = cur_offset + cdbp->cdb_map; - uschar * end_pos = end_offset + cdbp->cdb_map; - for (loop = 0; (loop < hash_offlen); ++loop) { - item_hash = cdb_unpack(cur_pos); - cur_pos += 4; - item_posn = cdb_unpack(cur_pos); - cur_pos += 4; - /* if the position is zero then we have a definite miss */ - if (item_posn == 0) - return FAIL; - - if (item_hash == key_hash) { - /* matching hash value */ - uschar * item_ptr = cdbp->cdb_map + item_posn; - item_key_len = cdb_unpack(item_ptr); - item_ptr += 4; - item_dat_len = cdb_unpack(item_ptr); - item_ptr += 4; - /* check key length matches */ - if (item_key_len == key_len) { - /* finally check if key matches */ - if (Ustrncmp(keystring, item_ptr, key_len) == 0) { - /* we have a match.... - * make item_ptr point to data */ - item_ptr += item_key_len; - /* ... and the returned result */ - *result = store_get(item_dat_len + 1); - memcpy(*result, item_ptr, item_dat_len); - (*result)[item_dat_len] = 0; - return OK; - } - } - } - /* handle warp round of table */ - if (cur_pos == end_pos) - cur_pos = cdbp->cdb_map + hash_offset; - } - /* looks like we failed... */ - return FAIL; - } -#endif /* HAVE_MMAP */ - for (loop = 0; (loop < hash_offlen); ++loop) { - uschar packbuf[8]; - if (lseek(cdbp->fileno, (off_t) cur_offset,SEEK_SET) == -1) return DEFER; - if (cdb_bread(cdbp->fileno, packbuf,8) == -1) return DEFER; - item_hash = cdb_unpack(packbuf); - item_posn = cdb_unpack(packbuf + 4); +/* make sure the mmap was OK */ +if (cdbp->cdb_map != NULL) + { + uschar * cur_pos = cur_offset + cdbp->cdb_map; + uschar * end_pos = end_offset + cdbp->cdb_map; + + for (loop = 0; (loop < hash_offlen); ++loop) + { + item_hash = cdb_unpack(cur_pos); + cur_pos += 4; + item_posn = cdb_unpack(cur_pos); + cur_pos += 4; + /* if the position is zero then we have a definite miss */ + if (item_posn == 0) return FAIL; - if (item_hash == key_hash) { - /* matching hash value */ - if (lseek(cdbp->fileno, (off_t) item_posn, SEEK_SET) == -1) return DEFER; - if (cdb_bread(cdbp->fileno, packbuf, 8) == -1) return DEFER; - item_key_len = cdb_unpack(packbuf); + if (item_hash == key_hash) + { /* matching hash value */ + uschar * item_ptr = cdbp->cdb_map + item_posn; + + item_key_len = cdb_unpack(item_ptr); + item_ptr += 4; + item_dat_len = cdb_unpack(item_ptr); + item_ptr += 4; + /* check key length matches */ - if (item_key_len == key_len) { - /* finally check if key matches */ - uschar * item_key = store_get(key_len); - if (cdb_bread(cdbp->fileno, item_key, key_len) == -1) return DEFER; - if (Ustrncmp(keystring, item_key, key_len) == 0) { - /* Reclaim some store */ - store_reset(item_key); - /* matches - get data length */ - item_dat_len = cdb_unpack(packbuf + 4); - /* then we build a new result string */ - *result = store_get(item_dat_len + 1); - if (cdb_bread(cdbp->fileno, *result, item_dat_len) == -1) - return DEFER; - (*result)[item_dat_len] = 0; - return OK; - } + + if (item_key_len == key_len) + { + /* finally check if key matches */ + if (Ustrncmp(keystring, item_ptr, key_len) == 0) + { + /* we have a match.... * make item_ptr point to data */ + + item_ptr += item_key_len; + + /* ... and the returned result */ + + *result = store_get(item_dat_len + 1); + memcpy(*result, item_ptr, item_dat_len); + (*result)[item_dat_len] = 0; + return OK; + } + } + } + /* handle warp round of table */ + if (cur_pos == end_pos) + cur_pos = cdbp->cdb_map + hash_offset; + } + /* looks like we failed... */ + return FAIL; + } + +#endif /* HAVE_MMAP */ + +for (loop = 0; (loop < hash_offlen); ++loop) + { + uschar packbuf[8]; + + if (lseek(cdbp->fileno, (off_t) cur_offset,SEEK_SET) == -1) return DEFER; + if (cdb_bread(cdbp->fileno, packbuf,8) == -1) return DEFER; + + item_hash = cdb_unpack(packbuf); + item_posn = cdb_unpack(packbuf + 4); + + /* if the position is zero then we have a definite miss */ + + if (item_posn == 0) + return FAIL; + + if (item_hash == key_hash) + { /* matching hash value */ + if (lseek(cdbp->fileno, (off_t) item_posn, SEEK_SET) == -1) return DEFER; + if (cdb_bread(cdbp->fileno, packbuf, 8) == -1) return DEFER; + + item_key_len = cdb_unpack(packbuf); + + /* check key length matches */ + + if (item_key_len == key_len) + { /* finally check if key matches */ + uschar * item_key = store_get(key_len); + + if (cdb_bread(cdbp->fileno, item_key, key_len) == -1) return DEFER; + if (Ustrncmp(keystring, item_key, key_len) == 0) { + /* Reclaim some store */ store_reset(item_key); + + /* matches - get data length */ + item_dat_len = cdb_unpack(packbuf + 4); + + /* then we build a new result string. We know we have enough + memory so disable Coverity errors about the tainted item_dat_ken */ + + *result = store_get(item_dat_len + 1); + /* coverity[tainted_data] */ + if (cdb_bread(cdbp->fileno, *result, item_dat_len) == -1) + return DEFER; + + /* coverity[tainted_data] */ + (*result)[item_dat_len] = 0; + return OK; + } + /* Reclaim some store */ + store_reset(item_key); } } - cur_offset += 8; + cur_offset += 8; - /* handle warp round of table */ - if (cur_offset == end_offset) - cur_offset = hash_offset; + /* handle warp round of table */ + if (cur_offset == end_offset) + cur_offset = hash_offset; } - return FAIL; +return FAIL; } diff --git a/src/src/rda.c b/src/src/rda.c index 476f06050..5df361e31 100644 --- a/src/src/rda.c +++ b/src/src/rda.c @@ -474,11 +474,12 @@ rda_read_string(int fd, uschar **sp) int len; if (read(fd, &len, sizeof(int)) != sizeof(int)) return FALSE; -if (len == 0) *sp = NULL; else - { - *sp = store_get(len); - if (read(fd, *sp, len) != len) return FALSE; - } +if (len == 0) + *sp = NULL; +else + /* We know we have enough memory so disable the error on "len" */ + /* coverity[tainted_data] */ + if (read(fd, *sp = store_get(len), len) != len) return FALSE; return TRUE; } diff --git a/src/src/spool_in.c b/src/src/spool_in.c index 1dcae49bd..e1d6e3422 100644 --- a/src/src/spool_in.c +++ b/src/src/spool_in.c @@ -672,10 +672,12 @@ DEBUG(D_deliver) #endif /* COMPILE_UTILITY */ /* After reading the tree, the next line has not yet been read into the -buffer. It contains the count of recipients which follow on separate lines. */ +buffer. It contains the count of recipients which follow on separate lines. +Apply an arbitrary sanity check.*/ if (Ufgets(big_buffer, big_buffer_size, f) == NULL) goto SPOOL_READ_ERROR; -if (sscanf(CS big_buffer, "%d", &rcount) != 1) goto SPOOL_FORMAT_ERROR; +if (sscanf(CS big_buffer, "%d", &rcount) != 1 || rcount > 16384) + goto SPOOL_FORMAT_ERROR; #ifndef COMPILE_UTILITY DEBUG(D_deliver) debug_printf("recipients_count=%d\n", rcount); @@ -684,6 +686,10 @@ DEBUG(D_deliver) debug_printf("recipients_count=%d\n", rcount); recipients_list_max = rcount; recipients_list = store_get(rcount * sizeof(recipient_item)); +/* We sanitised the count and know we have enough memory, so disable +the Coverity error on recipients_count */ +/* coverity[tainted_data] */ + for (recipients_count = 0; recipients_count < rcount; recipients_count++) { int nn; diff --git a/src/src/transports/queuefile.c b/src/src/transports/queuefile.c index 25747b3ab..7f10706c1 100644 --- a/src/src/transports/queuefile.c +++ b/src/src/transports/queuefile.c @@ -116,29 +116,29 @@ if (link_file) op = US"linking"; s = dstpath; } +else /* use data copy */ + { + DEBUG(D_transport) debug_printf("%s transport, copying %s => %s\n", + tb->name, srcpath, dstpath); -/* use data copy */ - -DEBUG(D_transport) debug_printf("%s transport, copying %s => %s\n", - tb->name, srcpath, dstpath); - -if ( (s = dstpath, - (dstfd = openat(ddfd, CCS filename, O_RDWR|O_CREAT|O_EXCL, SPOOL_MODE)) - < 0 - ) - || is_hdr_file - && (s = srcpath, (srcfd = openat(sdfd, CCS filename, O_RDONLY)) < 0) - ) - op = US"opening"; + if ( (s = dstpath, + (dstfd = openat(ddfd, CCS filename, O_RDWR|O_CREAT|O_EXCL, SPOOL_MODE)) + < 0 + ) + || is_hdr_file + && (s = srcpath, (srcfd = openat(sdfd, CCS filename, O_RDONLY)) < 0) + ) + op = US"opening"; -else - if (s = dstpath, fchmod(dstfd, SPOOL_MODE) != 0) - op = US"setting perms on"; else - if (!copy_spool_file(dstfd, srcfd)) - op = US"creating"; + if (s = dstpath, fchmod(dstfd, SPOOL_MODE) != 0) + op = US"setting perms on"; else - return TRUE; + if (!copy_spool_file(dstfd, srcfd)) + op = US"creating"; + else + return TRUE; + } addr->basic_errno = errno; addr->message = string_sprintf("%s transport %s file: %s failed with error: %s", -- 2.30.2