[12/30] storagefile: Fix backing format \0 check

Message ID 16d9ed51f14f1c74afce25c58aeedd85b0ec6daa.1570482718.git.crobinso@redhat.com
State New
Headers show
Series
  • storagefile, security: qcow2 data_file support
Related show

Commit Message

Cole Robinson Oct. 7, 2019, 9:49 p.m.
>From qemu.git docs/interop/qcow2.txt


  == String header extensions ==

  Some header extensions (such as the backing file format name and
  the external data file name) are just a single string. In this case,
  the header extension length is the string length and the string is
  not '\0' terminated. (The header extension padding can make it look
  like a string is '\0' terminated, but neither is padding always
  necessary nor is there a guarantee that zero bytes are used
  for padding.)

So we shouldn't be checking for a \0 byte at the end of the backing
format section. I think in practice there always is a \0 but we
shouldn't depend on that.

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/util/virstoragefile.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Daniel Henrique Barboza Oct. 9, 2019, 9:34 p.m. | #1
On 10/7/19 6:49 PM, Cole Robinson wrote:
> >From qemu.git docs/interop/qcow2.txt

Here's the '>' again. I think this is something you're using to cite an
external source in the commit message. Is that it?


>
>    == String header extensions ==
>
>    Some header extensions (such as the backing file format name and
>    the external data file name) are just a single string. In this case,
>    the header extension length is the string length and the string is
>    not '\0' terminated. (The header extension padding can make it look
>    like a string is '\0' terminated, but neither is padding always
>    necessary nor is there a guarantee that zero bytes are used
>    for padding.)
>
> So we shouldn't be checking for a \0 byte at the end of the backing
> format section. I think in practice there always is a \0 but we
> shouldn't depend on that.
>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>   src/util/virstoragefile.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 8cd576a463..5a4e4b24ae 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,
>           case QCOW2_HDR_EXTENSION_END:
>               goto done;
>   
> -        case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
> -            if (buf[offset+len] != '\0')
> -                break;
> -            *backingFormat = virStorageFileFormatTypeFromString(buf+offset);
> +        case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
> +            VIR_AUTOFREE(char *) tmp = NULL;
> +            if (VIR_ALLOC_N(tmp, len + 1) < 0)
> +                return -1;
> +            memcpy(tmp, buf + offset, len);
> +            tmp[len] = '\0';
> +
> +            *backingFormat = virStorageFileFormatTypeFromString(tmp);
>               if (*backingFormat <= VIR_STORAGE_FILE_NONE)
>                   return -1;
>           }
> +        }

I suppose this extra brace is due to the new brace right after label that
you added up there. I'm probably being a purist here, but I'd rather make
an extra indent level in each 'case' statement of this switch just to avoid
this  braces right below each other. We have this style of switch 
statement in
the code, so I think it would be ok.


DHB


>   
>           offset += len;
>       }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Prívozník Oct. 11, 2019, 1:04 p.m. | #2
On 10/7/19 11:49 PM, Cole Robinson wrote:
>>From qemu.git docs/interop/qcow2.txt

> 

>    == String header extensions ==

> 

>    Some header extensions (such as the backing file format name and

>    the external data file name) are just a single string. In this case,

>    the header extension length is the string length and the string is

>    not '\0' terminated. (The header extension padding can make it look

>    like a string is '\0' terminated, but neither is padding always

>    necessary nor is there a guarantee that zero bytes are used

>    for padding.)

> 

> So we shouldn't be checking for a \0 byte at the end of the backing

> format section. I think in practice there always is a \0 but we

> shouldn't depend on that.

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>   src/util/virstoragefile.c | 13 +++++++++----

>   1 file changed, 9 insertions(+), 4 deletions(-)

> 

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c

> index 8cd576a463..5a4e4b24ae 100644

> --- a/src/util/virstoragefile.c

> +++ b/src/util/virstoragefile.c

> @@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,

>           case QCOW2_HDR_EXTENSION_END:

>               goto done;

>   

> -        case QCOW2_HDR_EXTENSION_BACKING_FORMAT:

> -            if (buf[offset+len] != '\0')

> -                break;

> -            *backingFormat = virStorageFileFormatTypeFromString(buf+offset);

> +        case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {

> +            VIR_AUTOFREE(char *) tmp = NULL;

> +            if (VIR_ALLOC_N(tmp, len + 1) < 0)

> +                return -1;

> +            memcpy(tmp, buf + offset, len);

> +            tmp[len] = '\0';

> +

> +            *backingFormat = virStorageFileFormatTypeFromString(tmp);

>               if (*backingFormat <= VIR_STORAGE_FILE_NONE)

>                   return -1;

>           }

> +        }

>   


Pre-existing, but there's missing 'break;' here. Since you're touching 
these lines, might be worth putting it here.

>           offset += len;

>       }

> 


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Oct. 11, 2019, 5:54 p.m. | #3
On 10/9/19 5:34 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 10/7/19 6:49 PM, Cole Robinson wrote:
>> >From qemu.git docs/interop/qcow2.txt
> 
> Here's the '>' again. I think this is something you're using to cite an
> external source in the commit message. Is that it?
> 
> 
>>
>>    == String header extensions ==
>>
>>    Some header extensions (such as the backing file format name and
>>    the external data file name) are just a single string. In this case,
>>    the header extension length is the string length and the string is
>>    not '\0' terminated. (The header extension padding can make it look
>>    like a string is '\0' terminated, but neither is padding always
>>    necessary nor is there a guarantee that zero bytes are used
>>    for padding.)
>>
>> So we shouldn't be checking for a \0 byte at the end of the backing
>> format section. I think in practice there always is a \0 but we
>> shouldn't depend on that.
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>   src/util/virstoragefile.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 8cd576a463..5a4e4b24ae 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,
>>           case QCOW2_HDR_EXTENSION_END:
>>               goto done;
>> -        case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
>> -            if (buf[offset+len] != '\0')
>> -                break;
>> -            *backingFormat = 
>> virStorageFileFormatTypeFromString(buf+offset);
>> +        case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
>> +            VIR_AUTOFREE(char *) tmp = NULL;
>> +            if (VIR_ALLOC_N(tmp, len + 1) < 0)
>> +                return -1;
>> +            memcpy(tmp, buf + offset, len);
>> +            tmp[len] = '\0';
>> +
>> +            *backingFormat = virStorageFileFormatTypeFromString(tmp);
>>               if (*backingFormat <= VIR_STORAGE_FILE_NONE)
>>                   return -1;
>>           }
>> +        }
> 
> I suppose this extra brace is due to the new brace right after label that
> you added up there. I'm probably being a purist here, but I'd rather make
> an extra indent level in each 'case' statement of this switch just to avoid
> this  braces right below each other. We have this style of switch 
> statement in
> the code, so I think it would be ok.

That's my personal inclination as well, but a 'git grep -A1 "switch ("' 
shows that the vast majority of switch statements don't indent the case. 
The weirdness with the bracket here is just a side effect of that.

I swapped the case blocks around so at least the brackets are on 
adjacent lines :)

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Oct. 11, 2019, 6:03 p.m. | #4
On 10/11/19 9:04 AM, Michal Privoznik wrote:
> On 10/7/19 11:49 PM, Cole Robinson wrote:
>>> From qemu.git docs/interop/qcow2.txt
>>
>>    == String header extensions ==
>>
>>    Some header extensions (such as the backing file format name and
>>    the external data file name) are just a single string. In this case,
>>    the header extension length is the string length and the string is
>>    not '\0' terminated. (The header extension padding can make it look
>>    like a string is '\0' terminated, but neither is padding always
>>    necessary nor is there a guarantee that zero bytes are used
>>    for padding.)
>>
>> So we shouldn't be checking for a \0 byte at the end of the backing
>> format section. I think in practice there always is a \0 but we
>> shouldn't depend on that.
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>   src/util/virstoragefile.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 8cd576a463..5a4e4b24ae 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,
>>           case QCOW2_HDR_EXTENSION_END:
>>               goto done;
>> -        case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
>> -            if (buf[offset+len] != '\0')
>> -                break;
>> -            *backingFormat = 
>> virStorageFileFormatTypeFromString(buf+offset);
>> +        case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
>> +            VIR_AUTOFREE(char *) tmp = NULL;
>> +            if (VIR_ALLOC_N(tmp, len + 1) < 0)
>> +                return -1;
>> +            memcpy(tmp, buf + offset, len);
>> +            tmp[len] = '\0';
>> +
>> +            *backingFormat = virStorageFileFormatTypeFromString(tmp);
>>               if (*backingFormat <= VIR_STORAGE_FILE_NONE)
>>                   return -1;
>>           }
>> +        }
> 
> Pre-existing, but there's missing 'break;' here. Since you're touching 
> these lines, might be worth putting it here.
> 

Thanks, will fix before pushing

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Christophe de Dinechin Oct. 17, 2019, 9:29 a.m. | #5
Daniel Henrique Barboza writes:

> On 10/7/19 6:49 PM, Cole Robinson wrote:

>> >From qemu.git docs/interop/qcow2.txt

>

> Here's the '>' again. I think this is something you're using to cite an

> external source in the commit message. Is that it?


No, that is the way email "protects" lines that begin with "From". You
rarely see it nowadays because the mail itself is encoded, but if you
use plain text and send in ASCII, this will happen.

See page 78 of "The Unix Haters Handbook" [1] for details

[1] https://web.mit.edu/~simsong/www/ugh.pdf

--
Cheers,
Christophe de Dinechin (IRC c3d)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8cd576a463..5a4e4b24ae 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -506,13 +506,18 @@  qcow2GetExtensions(const char *buf,
         case QCOW2_HDR_EXTENSION_END:
             goto done;
 
-        case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
-            if (buf[offset+len] != '\0')
-                break;
-            *backingFormat = virStorageFileFormatTypeFromString(buf+offset);
+        case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
+            VIR_AUTOFREE(char *) tmp = NULL;
+            if (VIR_ALLOC_N(tmp, len + 1) < 0)
+                return -1;
+            memcpy(tmp, buf + offset, len);
+            tmp[len] = '\0';
+
+            *backingFormat = virStorageFileFormatTypeFromString(tmp);
             if (*backingFormat <= VIR_STORAGE_FILE_NONE)
                 return -1;
         }
+        }
 
         offset += len;
     }