target-ppc: kvm: Fix memory overflow issue about strncat()
PATCH 1/1 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.
--- target-ppc/kvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
target-ppc/kvm.c +2 -2
@@ -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) {
Signature · 1 line
1.9.3
Quoted 5 lines · Chen Gang
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 <[email protected]>
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
Quoted 20 lines
--- target-ppc/kvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)target-ppc/kvm.c +2
-2@@ -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) {
Quoted 5 lines · Alexander Graf
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):
target-ppc/kvm.c +4 -4
@@ -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.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 5 lines · Alexander Graf
Quoted 4 lines · Chen Gang
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.
Quoted 8 lines
Quoted 1 line
Signed-off-by Chen Gang <[email protected]>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.
I'd just use snprintf() like this:
target-ppc/kvm.c +1 -2
@@ -1794,8 +1794,7 @@ 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)); +snprintf (buf +strlen (buf),sizeof (buf) -strlen (buf),"/%s" , propname); f =fopen (buf,"rb" );if (!f) {
the buffer is of size PATH_MAX, and we're looking at /proc filesystem where names should be rather short so we're extremly unlikely to hit this prob in practice, there's no need to dynamically allocate a buffer for this stuff.
(Or alternatively there's asprintf(), but still I think it is overkill).
I can apply the above if everyone agrees.
Thanks,
/mjt
Quoted 20 lines
Quoted 19 lines
target-ppc/kvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)target-ppc/kvm.c +2
-2@@ -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) {
Quoted 37 lines
Am 24.10.2014 um 09:49 schrieb Michael Tokarev <[email protected]>:
Quoted 6 lines · Alexander Graf
Quoted 5 lines · Chen Gang
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.Quoted 8 lines
Quoted 1 line
Signed-off-by Chen Gang <[email protected]>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.I'd just use
snprintf()like this:target-ppc/kvm.c +1
-2@@ -1794,8 +1794,7 @@ 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)); +snprintf (buf +strlen (buf),sizeof (buf) -strlen (buf),"/%s" , propname); f =fopen (buf,"rb" );if (!f) {
Quoted 3 lines
the buffer is of size
PATH_MAX, and we're looking at /proc filesystem where names should be rather short so we're extremly unlikely to hit this prob in practice, there's no need to dynamically allocate a buffer for this stuff.
I've already applied the fix to ppc-next.
Quoted 2 lines
(Or alternatively there's
asprintf(), but still I think it is overkill).
That one isn't portable iirc.
Quoted 2 lines
I can apply the above if everyone agrees.
No worries, it's all over already ;).
Alex
Quoted 32 lines · Michael Tokarev
Quoted 5 lines · Alexander Graf
Quoted 4 lines · Chen Gang
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.Quoted 8 lines
Quoted 1 line
Signed-off-by Chen Gang <[email protected]>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.I'd just use
snprintf()like this:target-ppc/kvm.c +1
-2@@ -1794,8 +1794,7 @@ 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)); +snprintf (buf +strlen (buf),sizeof (buf) -strlen (buf),"/%s" , propname); f =fopen (buf,"rb" );if (!f) {
Quoted 8 lines
the buffer is of size
PATH_MAX, and we're looking at /proc filesystem where names should be rather short so we're extremly unlikely to hit this prob in practice, there's no need to dynamically allocate a buffer for this stuff.(Or alternatively there's
asprintf(), but still I think it is overkill).I can apply the above if everyone agrees.
For me, what you said is reasonable, although I am not sure whether the patch v2 for g_strdup_printf() was already applied or not.
Thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed