* Re: [Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-13 14:47 ` Alexander Graf
0 siblings, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2014-10-13 14:47 UTC (permalink / raw)
To: Chen Gang, pbonzini; +Cc: qemu-trivial, qemu-ppc, qemu-devel, kvm
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.5i5j@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) {
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-13 14:47 ` Alexander Graf
0 siblings, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2014-10-13 14:47 UTC (permalink / raw)
To: Chen Gang, pbonzini; +Cc: qemu-ppc, kvm, qemu-devel, qemu-trivial
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.5i5j@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) {
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
2014-10-13 14:47 ` Alexander Graf
(?)
@ 2014-10-13 15:43 ` Chen Gang
-1 siblings, 0 replies; 16+ messages in thread
From: Chen Gang @ 2014-10-13 15:43 UTC (permalink / raw)
To: Alexander Graf, pbonzini; +Cc: qemu-trivial, qemu-ppc, qemu-devel, kvm
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-13 15:43 ` Chen Gang
0 siblings, 0 replies; 16+ messages in thread
From: Chen Gang @ 2014-10-13 15:43 UTC (permalink / raw)
To: Alexander Graf, pbonzini; +Cc: qemu-trivial, qemu-ppc, qemu-devel, kvm
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-13 15:43 ` Chen Gang
0 siblings, 0 replies; 16+ messages in thread
From: Chen Gang @ 2014-10-13 15:43 UTC (permalink / raw)
To: Alexander Graf, pbonzini; +Cc: qemu-ppc, kvm, qemu-devel, qemu-trivial
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
2014-10-13 14:47 ` Alexander Graf
@ 2014-10-24 7:49 ` Michael Tokarev
-1 siblings, 0 replies; 16+ messages in thread
From: Michael Tokarev @ 2014-10-24 7:49 UTC (permalink / raw)
To: Alexander Graf, Chen Gang, pbonzini
Cc: qemu-trivial, qemu-ppc, qemu-devel, kvm
On 10/13/2014 06:47 PM, Alexander Graf wrote:
> 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.5i5j@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.
I'd just use snprintf() like this:
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 9c23c6b..5eaa36c 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -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
>> 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) {
>>
>
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-24 7:49 ` Michael Tokarev
0 siblings, 0 replies; 16+ messages in thread
From: Michael Tokarev @ 2014-10-24 7:49 UTC (permalink / raw)
To: Alexander Graf, Chen Gang, pbonzini
Cc: qemu-trivial, qemu-ppc, qemu-devel, kvm
On 10/13/2014 06:47 PM, Alexander Graf wrote:
> 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.5i5j@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.
I'd just use snprintf() like this:
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 9c23c6b..5eaa36c 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -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
>> 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) {
>>
>
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
2014-10-24 7:49 ` [Qemu-devel] " Michael Tokarev
(?)
@ 2014-10-24 8:05 ` Alexander Graf
-1 siblings, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2014-10-24 8:05 UTC (permalink / raw)
To: Michael Tokarev
Cc: Chen Gang, kvm@vger.kernel.org, qemu-trivial@nongnu.org,
qemu-devel, qemu-ppc@nongnu.org, pbonzini@redhat.com
> Am 24.10.2014 um 09:49 schrieb Michael Tokarev <mjt@tls.msk.ru>:
>
>> On 10/13/2014 06:47 PM, Alexander Graf wrote:
>>> 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.5i5j@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.
>
> I'd just use snprintf() like this:
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..5eaa36c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -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.
I've already applied the fix to ppc-next.
>
> (Or alternatively there's asprintf(), but still I think it is overkill).
That one isn't portable iirc.
>
> I can apply the above if everyone agrees.
No worries, it's all over already ;).
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-24 8:05 ` Alexander Graf
0 siblings, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2014-10-24 8:05 UTC (permalink / raw)
To: Michael Tokarev
Cc: Chen Gang, kvm@vger.kernel.org, qemu-trivial@nongnu.org,
qemu-devel, qemu-ppc@nongnu.org, pbonzini@redhat.com
> Am 24.10.2014 um 09:49 schrieb Michael Tokarev <mjt@tls.msk.ru>:
>
>> On 10/13/2014 06:47 PM, Alexander Graf wrote:
>>> 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.5i5j@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.
>
> I'd just use snprintf() like this:
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..5eaa36c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -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.
I've already applied the fix to ppc-next.
>
> (Or alternatively there's asprintf(), but still I think it is overkill).
That one isn't portable iirc.
>
> I can apply the above if everyone agrees.
No worries, it's all over already ;).
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-24 8:05 ` Alexander Graf
0 siblings, 0 replies; 16+ messages in thread
From: Alexander Graf @ 2014-10-24 8:05 UTC (permalink / raw)
To: Michael Tokarev
Cc: Chen Gang, pbonzini@redhat.com, qemu-trivial@nongnu.org,
qemu-ppc@nongnu.org, qemu-devel, kvm@vger.kernel.org
> Am 24.10.2014 um 09:49 schrieb Michael Tokarev <mjt@tls.msk.ru>:
>
>> On 10/13/2014 06:47 PM, Alexander Graf wrote:
>>> 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.5i5j@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.
>
> I'd just use snprintf() like this:
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..5eaa36c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -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.
I've already applied the fix to ppc-next.
>
> (Or alternatively there's asprintf(), but still I think it is overkill).
That one isn't portable iirc.
>
> I can apply the above if everyone agrees.
No worries, it's all over already ;).
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
2014-10-24 7:49 ` [Qemu-devel] " Michael Tokarev
@ 2014-10-24 8:19 ` Chen Gang
-1 siblings, 0 replies; 16+ messages in thread
From: Chen Gang @ 2014-10-24 8:19 UTC (permalink / raw)
To: Michael Tokarev, Alexander Graf, pbonzini
Cc: qemu-trivial, qemu-ppc, qemu-devel, kvm
On 10/24/14 15:49, Michael Tokarev wrote:
> On 10/13/2014 06:47 PM, Alexander Graf wrote:
>> 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.5i5j@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.
>
> I'd just use snprintf() like this:
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..5eaa36c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -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.
>
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.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()
@ 2014-10-24 8:19 ` Chen Gang
0 siblings, 0 replies; 16+ messages in thread
From: Chen Gang @ 2014-10-24 8:19 UTC (permalink / raw)
To: Michael Tokarev, Alexander Graf, pbonzini
Cc: qemu-trivial, qemu-ppc, qemu-devel, kvm
On 10/24/14 15:49, Michael Tokarev wrote:
> On 10/13/2014 06:47 PM, Alexander Graf wrote:
>> 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.5i5j@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.
>
> I'd just use snprintf() like this:
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..5eaa36c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -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.
>
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.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 16+ messages in thread