Libguestfs - Jan 2014 - Re: [PATCH 2/7] lib: Use vk->len for string conversion

On Sat, Jan 11, 2014 at 12:12:47AM +0100, Hilko Bengen
wrote:
> ---
>  lib/value.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/lib/value.c b/lib/value.c
> index 65404d7..e700c84 100644
> --- a/lib/value.c
> +++ b/lib/value.c
> @@ -207,14 +207,8 @@ hivex_value_key (hive_h *h, hive_value_h value)
>    struct ntreg_vk_record *vk >      (struct ntreg_vk_record *) ((char
*) h->addr + value);
>  
> -  /* AFAIK the key is always plain ASCII, so no conversion to UTF-8 is
> -   * necessary.  However we do need to nul-terminate the string.
> -   */
> -  errno = 0;
> -  size_t len = hivex_value_key_len (h, value);
> -  if (len == 0 && errno != 0)
> -    return NULL;
>    size_t flags = le16toh (vk->flags);
> +  size_t len = le16toh (vk->name_len);
I think this effectively removes a check.  hivex_value_key_len
contains this code:

  size_t seg_len = block_len (h, value, NULL);
  if (sizeof (struct ntreg_vk_record) + len - 1 > seg_len) {
    SET_ERRNO (EFAULT, "key length is too long (%zu, %zu)", len,
seg_len);
    return 0;
  }

but after this change, this would no longer be run, so it would be
possible to overrun the registry, or at least overrun the end of a
block.

Is there a reason for this patch?  It seems like just an optimization.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Mon, Jan 13, 2014 at 01:17:52PM +0000, Richard W.M. Jones
wrote:
> Is there a reason for this patch?  It seems like just an optimization.
OK, I see that the reason is to avoid calling _hivex_utf8_strlen which
would fail for the name containing a \0 character.  However I still
think we need to keep that check.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

---
 lib/value.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/value.c b/lib/value.c
index 65404d7..7b2e9d3 100644
--- a/lib/value.c
+++ b/lib/value.c
@@ -207,14 +207,14 @@ hivex_value_key (hive_h *h, hive_value_h value)
   struct ntreg_vk_record *vk      (struct ntreg_vk_record *) ((char *)
h->addr + value);
 
-  /* AFAIK the key is always plain ASCII, so no conversion to UTF-8 is
-   * necessary.  However we do need to nul-terminate the string.
-   */
-  errno = 0;
-  size_t len = hivex_value_key_len (h, value);
-  if (len == 0 && errno != 0)
-    return NULL;
   size_t flags = le16toh (vk->flags);
+  size_t len = le16toh (vk->name_len);
+
+  size_t seg_len = block_len (h, value, NULL);
+  if (sizeof (struct ntreg_vk_record) + len - 1 > seg_len) {
+    SET_ERRNO (EFAULT, "key length is too long (%zu, %zu)", len,
seg_len);
+    return 0;
+  }
   if (flags & 0x01) {
     return _hivex_windows_latin1_to_utf8 (vk->name, len);
   } else {
-- 
1.8.5.2

* Richard W.M. Jones:
> On Mon, Jan 13, 2014 at 01:17:52PM +0000, Richard W.M. Jones wrote:
>> Is there a reason for this patch?  It seems like just an optimization.
>
> OK, I see that the reason is to avoid calling _hivex_utf8_strlen which
> would fail for the name containing a \0 character.  However I still
> think we need to keep that check.
You're right. I have pulled the check into hivex_value_key().

Cheers,
-Hilko