[Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
strncat() will append additional '\0' to destination buffer, so need additional 1 byte for it, or may cause memory overflow, just like other area within QEMU have done. Signed-off-by: Chen Gang gang.chen.5...@gmail.com --- target-ppc/kvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 9c23c6b..66e7ce5 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -1794,8 +1794,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) return -1; } -strncat(buf, /, sizeof(buf) - strlen(buf)); -strncat(buf, propname, sizeof(buf) - strlen(buf)); +strncat(buf, /, sizeof(buf) - strlen(buf) - 1); +strncat(buf, propname, sizeof(buf) - strlen(buf) - 1); f = fopen(buf, rb); if (!f) { -- 1.9.3
Re: [Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
On 13.10.14 16:36, Chen Gang wrote: strncat() will append additional '\0' to destination buffer, so need additional 1 byte for it, or may cause memory overflow, just like other area within QEMU have done. Signed-off-by: Chen Gang gang.chen.5...@gmail.com I agree with this patch. However, the code is pretty ugly - I'm sure it must've been me who wrote it :). Could you please instead rewrite it to use g_strdup_printf() rather than strncat()s? That way we resolve all string pitfalls automatically - and this code is not the fast path, so doing an extra memory allocation is ok. Alex --- target-ppc/kvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 9c23c6b..66e7ce5 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -1794,8 +1794,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) return -1; } -strncat(buf, /, sizeof(buf) - strlen(buf)); -strncat(buf, propname, sizeof(buf) - strlen(buf)); +strncat(buf, /, sizeof(buf) - strlen(buf) - 1); +strncat(buf, propname, sizeof(buf) - strlen(buf) - 1); f = fopen(buf, rb); if (!f) {
Re: [Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
On 10/13/14 22:47, Alexander Graf wrote: Could you please instead rewrite it to use g_strdup_printf() rather than strncat()s? That way we resolve all string pitfalls automatically - and this code is not the fast path, so doing an extra memory allocation is ok. I guess, it is a personal taste. For me, it may need additional variable for g_strdup_printf(), and not save code lines, but *sprintf() is more readable than str*cat(). The related code may like below (welcome any improvement for it): diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 66e7ce5..cea6a87 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -1782,7 +1782,7 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len) * format) */ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) { -char buf[PATH_MAX]; +char buf[PATH_MAX], *tmp; union { uint32_t v32; uint64_t v64; @@ -1794,10 +1794,10 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname) return -1; } -strncat(buf, /, sizeof(buf) - strlen(buf) - 1); -strncat(buf, propname, sizeof(buf) - strlen(buf) - 1); +tmp = g_strdup_printf(%s/%s, buf, propname); -f = fopen(buf, rb); +f = fopen(tmp, rb); +g_free(buf); if (!f) { return -1; } For me, it is really a personal taste, so if the maintainer feels the diff above is OK, I shall send patch v2 for it within 2 days. Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed