Patchwork [2/4] Add virtio disk identification support

login
register
mail settings
Submitter john cooper
Date March 25, 2010, 5:33 a.m.
Message ID <4BAAF58E.4050507@redhat.com>
Download mbox | patch
Permalink /patch/48493/
State New
Headers show

Comments

john cooper - March 25, 2010, 5:33 a.m.
Fix bug which truncated serial string to 8 bytes, nul terminate.

Signed-off-by: john cooper <john.cooper@redhat.com>
---
Anthony Liguori - June 3, 2010, 7:10 p.m.
On 03/25/2010 12:33 AM, john cooper wrote:
> Fix bug which truncated serial string to 8 bytes, nul terminate.
>
> Signed-off-by: john cooper<john.cooper@redhat.com>
> ---
>
> diff --git a/vl.c b/vl.c
> index d69250c..b74cbba 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1162,7 +1162,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>       dinfo->on_write_error = on_write_error;
>       dinfo->opts = opts;
>       if (serial)
> -        strncpy(dinfo->serial, serial, sizeof(serial));
> +        strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1);
>    

You need to explicitly add a null terminator.  Far better to just never 
use strncpy().

Regards,

Anthony Liguori

>       QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>
>       switch(type) {
>
>
john cooper - June 4, 2010, 6:34 a.m.
Anthony Liguori wrote:
> On 03/25/2010 12:33 AM, john cooper wrote:
>> Fix bug which truncated serial string to 8 bytes, nul terminate.
>>
>> Signed-off-by: john cooper<john.cooper@redhat.com>
>> ---
>>
>> diff --git a/vl.c b/vl.c
>> index d69250c..b74cbba 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1162,7 +1162,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>>       dinfo->on_write_error = on_write_error;
>>       dinfo->opts = opts;
>>       if (serial)
>> -        strncpy(dinfo->serial, serial, sizeof(serial));
>> +        strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1);
>>    
> 
> You need to explicitly add a null terminator.  Far better to just never
> use strncpy().

As previous this is a case where dinfo->serial[] is defined
as BLOCK_SERIAL_STRLEN + 1 bytes as an internal convenience.
Above the context of the patch here is a:

    dinfo = qemu_mallocz(sizeof(*dinfo));

which assures this will do as intended, namely copy all
potential BLOCK_SERIAL_STRLEN bytes and assure they are
nul terminated should the full length be present.

I didn't conjure up the existing logic but rather am 
trying to peacefully coexist with it.

-john

Patch

diff --git a/vl.c b/vl.c
index d69250c..b74cbba 100644
--- a/vl.c
+++ b/vl.c
@@ -1162,7 +1162,7 @@  DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     dinfo->on_write_error = on_write_error;
     dinfo->opts = opts;
     if (serial)
-        strncpy(dinfo->serial, serial, sizeof(serial));
+        strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1);
     QTAILQ_INSERT_TAIL(&drives, dinfo, next);
 
     switch(type) {