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&token=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
>From ba82c1a00b3f3c9e6296a8a8e811e8270dbd9ec7 Mon Sep 17 00:00:00 2001
From: Ander Juaristi <[email protected]>
Date: Sun, 13 Dec 2015 00:13:31 +0100
Subject: [PATCH] Fix potential leak
* src/hsts.c (hsts_store_open): close fp if open.
* src/ftp.c (getftp): fix compiler warning for unused variable.
---
src/ftp.c | 1 -
src/hsts.c | 4 +++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/ftp.c b/src/ftp.c
index 002842e..cc90c3d 100644
--- a/src/ftp.c
+++ b/src/ftp.c
@@ -322,7 +322,6 @@ 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 = NULL;
- struct_fstat st;
char *respline, *tms;
const char *user, *passwd, *tmrate;
int cmd = con->cmd;
diff --git a/src/hsts.c b/src/hsts.c
index 1583659..af7ade1 100644
--- a/src/hsts.c
+++ b/src/hsts.c
@@ -485,10 +485,12 @@ hsts_store_open (const char *filename)
if (fstat (fileno (fp), &st) == 0)
store->last_mtime = st.st_mtime;
- fclose (fp);
}
out:
+ if (fp)
+ fclose (fp);
+
return store;
}
--
2.1.4
