Thanks, Ander. I splitted your patch into two and pushed them.
Regards, Tim Am Sonntag, 13. Dezember 2015, 15:29:26 schrieb Ander Juaristi: > Hi, > > Sorry I came out late, but I've just spotted a code path where fp is not > closed. > > Regards, > - AJ > > On 12/10/2015 11:14 PM, Darshit Shah wrote: > > Looks good to me. And coverity shows two issues fixed. So I'll push it. > > > > On 12/09, Juaristi Álamos, Ander wrote: > >> Darshit, could you test if these fixes pass the Coverity tests? > >> I'm not particularly sure of the HSTS fix. > >> > >> Regards, > >> - AJ > >> > >> On Sun, 2015-12-06 at 22:45 +0100, Darshit Shah wrote: > >>> ---------- Forwarded message ---------- > >>> From: <[email protected]> > >>> Date: 6 December 2015 at 22:39 > >>> Subject: New Defects reported by Coverity Scan for GNU Wget > >>> To: [email protected] > >>> > >>> > >>> > >>> Hi, > >>> > >>> Please find the latest report on new defect(s) introduced to GNU Wget > >>> found with Coverity Scan. > >>> > >>> 6 new defect(s) introduced to GNU Wget found with Coverity Scan. > >>> > >>> > >>> New defect(s) Reported-by: Coverity Scan > >>> Showing 6 of 6 defect(s) > >>> > >>> > >>> ** CID 1341706: (RESOURCE_LEAK) > >>> /src/ftp.c: 1518 in getftp() > >>> /src/ftp.c: 1528 in getftp() > >>> /src/ftp.c: 1518 in getftp() > >>> /src/ftp.c: 1518 in getftp() > >>> > >>> > >>> ________________________________________________________________________ > >>> ________________________________ *** CID 1341706: (RESOURCE_LEAK) > >>> /src/ftp.c: 1518 in getftp() > >>> 1512 logputs (LOG_NOTQUIET, "Server does not want to > >>> resume the SSL session. Trying with a new one.\n"); > >>> 1513 if (!ssl_connect_wget (dtsock, u->host, NULL)) > >>> 1514 { > >>> 1515 fd_close (csock); > >>> 1516 fd_close (dtsock); > >>> 1517 logputs (LOG_NOTQUIET, "Could not perform SSL > >>> handshake.\n"); > >>> > >>> >>> CID 1341706: (RESOURCE_LEAK) > >>> >>> Variable "fp" going out of scope leaks the storage it points to. > >>> > >>> 1518 return CONERROR; > >>> 1519 } > >>> 1520 } > >>> 1521 else > >>> 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > >>> connection.\n"); > >>> 1523 > >>> /src/ftp.c: 1528 in getftp() > >>> 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > >>> connection.\n"); > >>> 1523 > >>> 1524 if (!ssl_check_certificate (dtsock, u->host)) > >>> 1525 { > >>> 1526 fd_close (csock); > >>> 1527 fd_close (dtsock); > >>> > >>> >>> CID 1341706: (RESOURCE_LEAK) > >>> >>> Variable "fp" going out of scope leaks the storage it points to. > >>> > >>> 1528 return CONERROR; > >>> 1529 } > >>> 1530 } > >>> 1531 #endif > >>> 1532 > >>> 1533 /* Get the contents of the document. */ > >>> /src/ftp.c: 1518 in getftp() > >>> 1512 logputs (LOG_NOTQUIET, "Server does not want to > >>> resume the SSL session. Trying with a new one.\n"); > >>> 1513 if (!ssl_connect_wget (dtsock, u->host, NULL)) > >>> 1514 { > >>> 1515 fd_close (csock); > >>> 1516 fd_close (dtsock); > >>> 1517 logputs (LOG_NOTQUIET, "Could not perform SSL > >>> handshake.\n"); > >>> > >>> >>> CID 1341706: (RESOURCE_LEAK) > >>> >>> Variable "fp" going out of scope leaks the storage it points to. > >>> > >>> 1518 return CONERROR; > >>> 1519 } > >>> 1520 } > >>> 1521 else > >>> 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > >>> connection.\n"); > >>> 1523 > >>> /src/ftp.c: 1518 in getftp() > >>> 1512 logputs (LOG_NOTQUIET, "Server does not want to > >>> resume the SSL session. Trying with a new one.\n"); > >>> 1513 if (!ssl_connect_wget (dtsock, u->host, NULL)) > >>> 1514 { > >>> 1515 fd_close (csock); > >>> 1516 fd_close (dtsock); > >>> 1517 logputs (LOG_NOTQUIET, "Could not perform SSL > >>> handshake.\n"); > >>> > >>> >>> CID 1341706: (RESOURCE_LEAK) > >>> >>> Variable "fp" going out of scope leaks the storage it points to. > >>> > >>> 1518 return CONERROR; > >>> 1519 } > >>> 1520 } > >>> 1521 else > >>> 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > >>> connection.\n"); > >>> 1523 > >>> > >>> ** CID 1341705: Security best practices violations (TOCTOU) > >>> /src/hsts.c: 479 in hsts_store_open() > >>> > >>> > >>> ________________________________________________________________________ > >>> ________________________________ *** CID 1341705: Security best > >>> practices violations (TOCTOU) > >>> /src/hsts.c: 479 in hsts_store_open() > >>> 473 > >>> 474 if (file_exists_p (filename)) > >>> 475 { > >>> 476 if (stat (filename, &st) == 0) > >>> 477 store->last_mtime = st.st_mtime; > >>> 478 > >>> > >>> >>> CID 1341705: Security best practices violations (TOCTOU) > >>> >>> Calling function "fopen" that uses "filename" after a check > >>> >>> function. This can cause a time-of-check, time-of-use race > >>> >>> condition.>>> > >>> 479 fp = fopen (filename, "r"); > >>> 480 if (!fp || !hsts_read_database (store, fp, false)) > >>> 481 { > >>> 482 /* abort! */ > >>> 483 hsts_store_close (store); > >>> 484 xfree (store); > >>> > >>> ** CID 1273467: API usage errors (BUFFER_SIZE) > >>> /lib/md5.c: 291 in md5_process_bytes() > >>> > >>> > >>> ________________________________________________________________________ > >>> ________________________________ *** CID 1273467: API usage errors > >>> (BUFFER_SIZE) > >>> /lib/md5.c: 291 in md5_process_bytes() > >>> 285 memcpy (&((char *) ctx->buffer)[left_over], buffer, len); > >>> 286 left_over += len; > >>> 287 if (left_over >= 64) > >>> 288 { > >>> 289 md5_process_block (ctx->buffer, 64, ctx); > >>> 290 left_over -= 64; > >>> > >>> >>> CID 1273467: API usage errors (BUFFER_SIZE) > >>> >>> The source buffer "&ctx->buffer[16]" potentially overlaps with > >>> >>> the destination buffer "ctx->buffer", which results in > >>> >>> undefined behavior for memcpy.>>> > >>> 291 memcpy (ctx->buffer, &ctx->buffer[16], left_over); > >>> 292 } > >>> 293 ctx->buflen = left_over; > >>> 294 } > >>> 295 } > >>> 296 > >>> > >>> ** CID 1273466: API usage errors (BUFFER_SIZE) > >>> /lib/sha256.c: 411 in sha256_process_bytes() > >>> > >>> > >>> ________________________________________________________________________ > >>> ________________________________ *** CID 1273466: API usage errors > >>> (BUFFER_SIZE) > >>> /lib/sha256.c: 411 in sha256_process_bytes() > >>> 405 memcpy (&((char *) ctx->buffer)[left_over], buffer, len); > >>> 406 left_over += len; > >>> 407 if (left_over >= 64) > >>> 408 { > >>> 409 sha256_process_block (ctx->buffer, 64, ctx); > >>> 410 left_over -= 64; > >>> > >>> >>> CID 1273466: API usage errors (BUFFER_SIZE) > >>> >>> The source buffer "&ctx->buffer[16]" potentially overlaps with > >>> >>> the destination buffer "ctx->buffer", which results in > >>> >>> undefined behavior for memcpy.>>> > >>> 411 memcpy (ctx->buffer, &ctx->buffer[16], left_over); > >>> 412 } > >>> 413 ctx->buflen = left_over; > >>> 414 } > >>> 415 } > >>> 416 > >>> > >>> ** CID 1273463: API usage errors (BUFFER_SIZE) > >>> /lib/sha1.c: 278 in sha1_process_bytes() > >>> > >>> > >>> ________________________________________________________________________ > >>> ________________________________ *** CID 1273463: API usage errors > >>> (BUFFER_SIZE) > >>> /lib/sha1.c: 278 in sha1_process_bytes() > >>> 272 memcpy (&((char *) ctx->buffer)[left_over], buffer, len); > >>> 273 left_over += len; > >>> 274 if (left_over >= 64) > >>> 275 { > >>> 276 sha1_process_block (ctx->buffer, 64, ctx); > >>> 277 left_over -= 64; > >>> > >>> >>> CID 1273463: API usage errors (BUFFER_SIZE) > >>> >>> The source buffer "&ctx->buffer[16]" potentially overlaps with > >>> >>> the destination buffer "ctx->buffer", which results in > >>> >>> undefined behavior for memcpy.>>> > >>> 278 memcpy (ctx->buffer, &ctx->buffer[16], left_over); > >>> 279 } > >>> 280 ctx->buflen = left_over; > >>> 281 } > >>> 282 } > >>> 283 > >>> > >>> ** CID 420711: Insecure data handling (INTEGER_OVERFLOW) > >>> /lib/str-two-way.h: 221 in critical_factorization() > >>> > >>> > >>> ________________________________________________________________________ > >>> ________________________________ *** CID 420711: Insecure data handling > >>> (INTEGER_OVERFLOW) > >>> /lib/str-two-way.h: 221 in critical_factorization() > >>> 215 lexicographic suffix of 'a' works for 'bba', but not 'ab' > >>> for > >>> 216 'aab'. The shorter suffix of the two will always be a > >>> critical 217 factorization. */ > >>> 218 if (max_suffix_rev + 1 < max_suffix + 1) > >>> 219 return max_suffix + 1; > >>> 220 *period = p; > >>> > >>> >>> CID 420711: Insecure data handling (INTEGER_OVERFLOW) > >>> >>> Overflowed or truncated value (or a value computed from an > >>> >>> overflowed or truncated value) "max_suffix_rev + 1UL" used as > >>> >>> return value.>>> > >>> 221 return max_suffix_rev + 1; > >>> 222 } > >>> 223 > >>> 224 /* Return the first location of non-empty NEEDLE within > >>> HAYSTACK, or 225 NULL. HAYSTACK_LEN is the minimum known length > >>> of HAYSTACK. This 226 method is optimized for NEEDLE_LEN < > >>> LONG_NEEDLE_THRESHOLD. > >>> > >>> > >>> ________________________________________________________________________ > >>> ________________________________ To view the defects in Coverity Scan > >>> visit, > >>> https://scan.coverity.com/projects/gnu-wget?tab=overview > >>> > >>> To manage Coverity Scan email notifications for "[email protected]", > >>> click > >>> https://scan.coverity.com/subscriptions/edit?email=darnir%40gmail.com&t > >>> oken=a247cf0e017fe1ea3e52680a7e0c1fcf>> > >> From c78aee99ba099a61af26c9df4c072e6e7a65cb03 Mon Sep 17 00:00:00 2001 > >> From: Ander Juaristi <[email protected]> > >> Date: Wed, 9 Dec 2015 17:12:51 +0100 > >> Subject: [PATCH] Fix Coverity issues > >> > >> * src/ftp.c (getftp): on error, close the file and attempt to remove it > >> > >> before exiting. > >> > >> * src/hsts.c (hsts_store_open): update modification time in the end. > >> --- > >> src/ftp.c | 16 +++++++++++++--- > >> src/hsts.c | 16 +++++++++------- > >> 2 files changed, 22 insertions(+), 10 deletions(-) > >> > >> diff --git a/src/ftp.c b/src/ftp.c > >> index 5394b71..002842e 100644 > >> --- a/src/ftp.c > >> +++ b/src/ftp.c > >> @@ -321,7 +321,8 @@ getftp (struct url *u, wgint passed_expected_bytes, > >> wgint *qtyread, { > >> > >> int csock, dtsock, local_sock, res; > >> uerr_t err = RETROK; /* appease the compiler */ > >> > >> - FILE *fp; > >> + FILE *fp = NULL; > >> + struct_fstat st; > >> > >> char *respline, *tms; > >> const char *user, *passwd, *tmrate; > >> int cmd = con->cmd; > >> > >> @@ -1514,8 +1515,9 @@ Error in server response, closing control > >> connection.\n"));>> > >> { > >> > >> fd_close (csock); > >> fd_close (dtsock); > >> > >> + err = CONERROR; > >> > >> logputs (LOG_NOTQUIET, "Could not perform SSL > >> handshake.\n"); > >> > >> - return CONERROR; > >> + goto exit_error; > >> > >> } > >> > >> } > >> > >> else > >> > >> @@ -1525,7 +1527,8 @@ Error in server response, closing control > >> connection.\n"));>> > >> { > >> > >> fd_close (csock); > >> fd_close (dtsock); > >> > >> - return CONERROR; > >> + err = CONERROR; > >> + goto exit_error; > >> > >> } > >> > >> } > >> > >> #endif > >> @@ -1762,6 +1765,13 @@ Error in server response, closing control > >> connection.\n"));>> > >> } > >> > >> } while (try_again); > >> return RETRFINISHED; > >> > >> + > >> +exit_error: > >> + > >> + /* If fp is a regular file, close and try to remove it */ > >> + if (fp && !output_stream) > >> + fclose (fp); > >> + return err; > >> } > >> > >> /* A one-file FTP loop. This is the part where FTP retrieval is > >> diff --git a/src/hsts.c b/src/hsts.c > >> index 3ddbf72..1583659 100644 > >> --- a/src/hsts.c > >> +++ b/src/hsts.c > >> @@ -464,7 +464,7 @@ hsts_store_t > >> hsts_store_open (const char *filename) > >> { > >> > >> hsts_store_t store = NULL; > >> > >> - struct stat st; > >> + struct_stat st; > >> > >> FILE *fp = NULL; > >> > >> store = xnew0 (struct hsts_store); > >> > >> @@ -473,27 +473,29 @@ hsts_store_open (const char *filename) > >> > >> if (file_exists_p (filename)) > >> > >> { > >> > >> - if (stat (filename, &st) == 0) > >> - store->last_mtime = st.st_mtime; > >> - > >> > >> fp = fopen (filename, "r"); > >> > >> + > >> > >> if (!fp || !hsts_read_database (store, fp, false)) > >> > >> { > >> > >> /* abort! */ > >> hsts_store_close (store); > >> xfree (store); > >> > >> + goto out; > >> > >> } > >> > >> - if (fp) > >> - fclose (fp); > >> + > >> + if (fstat (fileno (fp), &st) == 0) > >> + store->last_mtime = st.st_mtime; > >> + fclose (fp); > >> > >> } > >> > >> +out: > >> return store; > >> > >> } > >> > >> void > >> hsts_store_save (hsts_store_t store, const char *filename) > >> { > >> - struct stat st; > >> + struct_stat st; > >> > >> FILE *fp = NULL; > >> int fd = 0; > >> > >> -- > >> 2.1.4
