Tim Rühsen wrote:
> I could invite you (or anybody else) to view the detected defects. Just
> give me an OK and/or your email address before I do so.
>
> From what I can quickly see, some defects might be quite serious.
What I can see (in the limited set of gnulib modules that wget uses):
1) Coverity suggests to use 'memmove' instead of 'memcpy' in a couple
of places where it cannot prove that the source and destination memory
regions don't overlap.
I don't know what could make the coverity warnings disappear, but at least
we can add comments that help us verify that there is no issue. Invariants,
as usual.
2) A false warning at
len >> 31 >> 31 >> 2
because the code is prepared for 128-bit integers but coverity "knows" that
'len' is at most 64 bits wide.
3) malloc/free related warnings in glob.c.
Please review the three attached patches.
From 3e7178b337e3a83df9e4f36a4aef516f089e3796 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 31 Mar 2017 22:03:49 +0200
Subject: [PATCH 1/3] md5, sha1, sha256, sha512: Add comments regarding
correctness.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/md5.h (buflen): Add comments regarding range.
* lib/sha1.h (buflen): Likewise.
* lib/sha256.h (buflen): Likewise.
* lib/sha512.h (buflen): Likewise.
* lib/md5.c (md5_process_bytes): Add comment why memmove is not needed.
* lib/sha1.c (sha1_process_bytes): Likewise.
* lib/sha256.c (sha256_process_bytes): Likewise.
* lib/sha512.c (sha512_process_bytes): Likewise.
Reported by Coverity via Tim Rühsen.
---
ChangeLog | 13 +++++++++++++
lib/md5.c | 5 ++++-
lib/md5.h | 4 ++--
lib/sha1.c | 5 ++++-
lib/sha1.h | 4 ++--
lib/sha256.c | 5 ++++-
lib/sha256.h | 4 ++--
lib/sha512.c | 5 ++++-
lib/sha512.h | 4 ++--
9 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index ad7ae9e..7c15292 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2017-03-31 Bruno Haible <br...@clisp.org>
+
+ md5, sha1, sha256, sha512: Add comments regarding correctness.
+ * lib/md5.h (buflen): Add comments regarding range.
+ * lib/sha1.h (buflen): Likewise.
+ * lib/sha256.h (buflen): Likewise.
+ * lib/sha512.h (buflen): Likewise.
+ * lib/md5.c (md5_process_bytes): Add comment why memmove is not needed.
+ * lib/sha1.c (sha1_process_bytes): Likewise.
+ * lib/sha256.c (sha256_process_bytes): Likewise.
+ * lib/sha512.c (sha512_process_bytes): Likewise.
+ Reported by Coverity via Tim Rühsen.
+
2017-03-22 Paul Eggert <egg...@cs.ucla.edu>
getopt: merge from glibc
diff --git a/lib/md5.c b/lib/md5.c
index 9f5237e..8650957 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -246,7 +246,8 @@ md5_process_bytes (const void *buffer, size_t len, struct md5_ctx *ctx)
md5_process_block (ctx->buffer, ctx->buflen & ~63, ctx);
ctx->buflen &= 63;
- /* The regions in the following copy operation cannot overlap. */
+ /* The regions in the following copy operation cannot overlap,
+ because ctx->buflen < 64 ≤ (left_over + add) & ~63. */
memcpy (ctx->buffer,
&((char *) ctx->buffer)[(left_over + add) & ~63],
ctx->buflen);
@@ -288,6 +289,8 @@ md5_process_bytes (const void *buffer, size_t len, struct md5_ctx *ctx)
{
md5_process_block (ctx->buffer, 64, ctx);
left_over -= 64;
+ /* The regions in the following copy operation cannot overlap,
+ because left_over ≤ 64. */
memcpy (ctx->buffer, &ctx->buffer[16], left_over);
}
ctx->buflen = left_over;
diff --git a/lib/md5.h b/lib/md5.h
index 9c2c098..543a366 100644
--- a/lib/md5.h
+++ b/lib/md5.h
@@ -74,8 +74,8 @@ struct md5_ctx
uint32_t D;
uint32_t total[2];
- uint32_t buflen;
- uint32_t buffer[32];
+ uint32_t buflen; /* ≥ 0, ≤ 128 */
+ uint32_t buffer[32]; /* 128 bytes; the first buflen bytes are in use */
};
/*
diff --git a/lib/sha1.c b/lib/sha1.c
index 87c5771..6908650 100644
--- a/lib/sha1.c
+++ b/lib/sha1.c
@@ -233,7 +233,8 @@ sha1_process_bytes (const void *buffer, size_t len, struct sha1_ctx *ctx)
sha1_process_block (ctx->buffer, ctx->buflen & ~63, ctx);
ctx->buflen &= 63;
- /* The regions in the following copy operation cannot overlap. */
+ /* The regions in the following copy operation cannot overlap,
+ because ctx->buflen < 64 ≤ (left_over + add) & ~63. */
memcpy (ctx->buffer,
&((char *) ctx->buffer)[(left_over + add) & ~63],
ctx->buflen);
@@ -275,6 +276,8 @@ sha1_process_bytes (const void *buffer, size_t len, struct sha1_ctx *ctx)
{
sha1_process_block (ctx->buffer, 64, ctx);
left_over -= 64;
+ /* The regions in the following copy operation cannot overlap,
+ because left_over ≤ 64. */
memcpy (ctx->buffer, &ctx->buffer[16], left_over);
}
ctx->buflen = left_over;
diff --git a/lib/sha1.h b/lib/sha1.h
index 38e82f3..0deb7ba 100644
--- a/lib/sha1.h
+++ b/lib/sha1.h
@@ -46,8 +46,8 @@ struct sha1_ctx
uint32_t E;
uint32_t total[2];
- uint32_t buflen;
- uint32_t buffer[32];
+ uint32_t buflen; /* ≥ 0, ≤ 128 */
+ uint32_t buffer[32]; /* 128 bytes; the first buflen bytes are in use */
};
/* Initialize structure containing state of computation. */
diff --git a/lib/sha256.c b/lib/sha256.c
index 03d3899..c0fb8be 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -366,7 +366,8 @@ sha256_process_bytes (const void *buffer, size_t len, struct sha256_ctx *ctx)
sha256_process_block (ctx->buffer, ctx->buflen & ~63, ctx);
ctx->buflen &= 63;
- /* The regions in the following copy operation cannot overlap. */
+ /* The regions in the following copy operation cannot overlap,
+ because ctx->buflen < 64 ≤ (left_over + add) & ~63. */
memcpy (ctx->buffer,
&((char *) ctx->buffer)[(left_over + add) & ~63],
ctx->buflen);
@@ -408,6 +409,8 @@ sha256_process_bytes (const void *buffer, size_t len, struct sha256_ctx *ctx)
{
sha256_process_block (ctx->buffer, 64, ctx);
left_over -= 64;
+ /* The regions in the following copy operation cannot overlap,
+ because left_over ≤ 64. */
memcpy (ctx->buffer, &ctx->buffer[16], left_over);
}
ctx->buflen = left_over;
diff --git a/lib/sha256.h b/lib/sha256.h
index ffb91fa..348b76e 100644
--- a/lib/sha256.h
+++ b/lib/sha256.h
@@ -44,8 +44,8 @@ struct sha256_ctx
uint32_t state[8];
uint32_t total[2];
- size_t buflen;
- uint32_t buffer[32];
+ size_t buflen; /* ≥ 0, ≤ 128 */
+ uint32_t buffer[32]; /* 128 bytes; the first buflen bytes are in use */
};
/* Initialize structure containing state of computation. */
diff --git a/lib/sha512.c b/lib/sha512.c
index 6876bfd..dbde671 100644
--- a/lib/sha512.c
+++ b/lib/sha512.c
@@ -374,7 +374,8 @@ sha512_process_bytes (const void *buffer, size_t len, struct sha512_ctx *ctx)
sha512_process_block (ctx->buffer, ctx->buflen & ~127, ctx);
ctx->buflen &= 127;
- /* The regions in the following copy operation cannot overlap. */
+ /* The regions in the following copy operation cannot overlap,
+ because ctx->buflen < 128 ≤ (left_over + add) & ~127. */
memcpy (ctx->buffer,
&((char *) ctx->buffer)[(left_over + add) & ~127],
ctx->buflen);
@@ -416,6 +417,8 @@ sha512_process_bytes (const void *buffer, size_t len, struct sha512_ctx *ctx)
{
sha512_process_block (ctx->buffer, 128, ctx);
left_over -= 128;
+ /* The regions in the following copy operation cannot overlap,
+ because left_over ≤ 128. */
memcpy (ctx->buffer, &ctx->buffer[16], left_over);
}
ctx->buflen = left_over;
diff --git a/lib/sha512.h b/lib/sha512.h
index 121e6c3..4460e6c 100644
--- a/lib/sha512.h
+++ b/lib/sha512.h
@@ -44,8 +44,8 @@ struct sha512_ctx
u64 state[8];
u64 total[2];
- size_t buflen;
- u64 buffer[32];
+ size_t buflen; /* ≥ 0, ≤ 256 */
+ u64 buffer[32]; /* 256 bytes; the first buflen bytes are in use */
};
/* Initialize structure containing state of computation. */
--
2.7.4
From 0eee3ccaae5bb3d0016a0da8b8e5108767c02748 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 31 Mar 2017 22:41:38 +0200
Subject: [PATCH 2/3] glob: Fix memory leaks.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/glob.c (glob): Free allocated memory before returning.
Reported by Coverity via Tim Rühsen.
---
ChangeLog | 6 ++++++
lib/glob.c | 13 +++++++++++--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 7c15292..cca7542 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
2017-03-31 Bruno Haible <br...@clisp.org>
+ glob: Fix memory leaks.
+ * lib/glob.c (glob): Free allocated memory before returning.
+ Reported by Coverity via Tim Rühsen.
+
+2017-03-31 Bruno Haible <br...@clisp.org>
+
md5, sha1, sha256, sha512: Add comments regarding correctness.
* lib/md5.h (buflen): Add comments regarding range.
* lib/sha1.h (buflen): Likewise.
diff --git a/lib/glob.c b/lib/glob.c
index e69a96e..1337817 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -789,10 +789,14 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
malloc_home_dir = 1;
}
memcpy (home_dir, p->pw_dir, home_dir_len);
-
- free (pwtmpbuf);
}
}
+ free (malloc_pwtmpbuf);
+ }
+ else
+ {
+ if (__glibc_unlikely (malloc_name))
+ free (name);
}
}
if (home_dir == NULL || home_dir[0] == '\0')
@@ -847,6 +851,9 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
dirname = newp;
dirlen += home_len - 1;
malloc_dirname = !use_alloca;
+
+ if (__glibc_unlikely (malloc_home_dir))
+ free (home_dir);
}
dirname_modified = 1;
}
@@ -1048,6 +1055,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
if (newcount > SIZE_MAX / sizeof (char *) - 2)
{
nospace:
+ if (__glibc_unlikely (malloc_dirname))
+ free (dirname);
free (pglob->gl_pathv);
pglob->gl_pathv = NULL;
pglob->gl_pathc = 0;
--
2.7.4
From b1d7f3165ba1c7a44a29017eb80491094aa240ba Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 31 Mar 2017 22:43:35 +0200
Subject: [PATCH 3/3] glob: Fix invalid free() call.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/glob.c (glob): Reset malloc_home_dir when assigning a pointer to
static storage to home_dir.
Reported by Coverity via Tim Rühsen.
---
ChangeLog | 7 +++++++
lib/glob.c | 5 ++++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index cca7542..2e01161 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
2017-03-31 Bruno Haible <br...@clisp.org>
+ glob: Fix invalid free() call.
+ * lib/glob.c (glob): Reset malloc_home_dir when assigning a pointer to
+ static storage to home_dir.
+ Reported by Coverity via Tim Rühsen.
+
+2017-03-31 Bruno Haible <br...@clisp.org>
+
glob: Fix memory leaks.
* lib/glob.c (glob): Free allocated memory before returning.
Reported by Coverity via Tim Rühsen.
diff --git a/lib/glob.c b/lib/glob.c
index 1337817..9305038 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -809,7 +809,10 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
goto out;
}
else
- home_dir = (char *) "~"; /* No luck. */
+ {
+ home_dir = (char *) "~"; /* No luck. */
+ malloc_home_dir = 0;
+ }
}
# endif /* WINDOWS32 */
# endif
--
2.7.4
