diff mbox

[ovs-dev] Windows: Fix wmi.c to use count of wchar_t instead of sizeof

Message ID 20170127055750.8384-1-vsairam@vmware.com
State Superseded
Headers show

Commit Message

Sairam Venugopal Jan. 27, 2017, 5:57 a.m. UTC 
wcscat_s and wcscpy_s requires number of elements as argument. wchar_t
uses 2 bytes for storage and using sizeof(internal_port_query) causes
access violation error on Windows 2012 R2 (64 bit). This patch introduces
a count_of() function to return the count of the wchar_t array.

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
Reported-by: Sairam Venugopal <vsairam@vmware.com>
Reported-at: openvswitch/ovs-issues#121
---
 lib/wmi.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Comments

Alin Serdean Jan. 27, 2017, 6:57 a.m. UTC | #1 
Thanks for catching this!

It slipped my mind when I wrote the code.

Mind adding a define for the number of element instead and passing that in?
i.e.
#define WMI_QUERY 2048
    wchar_t internal_port_query[WMI_QUERY] = L"SELECT * from "
        L"Msvm_EthernetPortAllocationSettingData  WHERE ElementName = \"" ; 
....
wcscat_s(internal_port_query, WMI_QUERY, wide_name);
instead?
(I am using for both delete/create and maybe we want to increase its size)

Also another thing that I forgot is to check the output of wcscat_s/wcscpy_s. In only managed to check it once in 'get_str_value' :(.

Should I send an incremental?

Thanks,
Alin.

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Sairam Venugopal
> Sent: Friday, January 27, 2017 7:58 AM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] Windows: Fix wmi.c to use count of wchar_t
> instead of sizeof
> 
> wcscat_s and wcscpy_s requires number of elements as argument. wchar_t
> uses 2 bytes for storage and using sizeof(internal_port_query) causes access
> violation error on Windows 2012 R2 (64 bit). This patch introduces a
> count_of() function to return the count of the wchar_t array.
> 
> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
> Reported-by: Sairam Venugopal <vsairam@vmware.com>
> Reported-at: openvswitch/ovs-issues#121
> ---
>  lib/wmi.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
Sairam Venugopal Jan. 27, 2017, 7:38 a.m. UTC | #2 
Yes, my initial thought was to define 2048 and re-use it. I added in the define in case we use something other than 2048 in the future.

I am okay with either one of the approaches. I can send out a new patch with - #define WMI_QUERY_ARRAY_SIZE set to 2048.

We can do an incremental for checking the output of wcscat_s/wcscpy_s. However, I noticed that the application would crash instead of returning an errno_t. When the size was large it gave an access violation and when it was small it just crashed with an error finding null pointer.

Thanks,
Sairam




On 1/26/17, 10:57 PM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote:

>Thanks for catching this!
>
>It slipped my mind when I wrote the code.
>
>Mind adding a define for the number of element instead and passing that in?
>i.e.
>#define WMI_QUERY 2048
>    wchar_t internal_port_query[WMI_QUERY] = L"SELECT * from "
>        L"Msvm_EthernetPortAllocationSettingData  WHERE ElementName = \"" ; 
>....
>wcscat_s(internal_port_query, WMI_QUERY, wide_name);
>instead?
>(I am using for both delete/create and maybe we want to increase its size)
>
>Also another thing that I forgot is to check the output of wcscat_s/wcscpy_s. In only managed to check it once in 'get_str_value' :(.
>
>Should I send an incremental?
>
>Thanks,
>Alin.
>
>> -----Original Message-----
>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
>> bounces@openvswitch.org] On Behalf Of Sairam Venugopal
>> Sent: Friday, January 27, 2017 7:58 AM
>> To: dev@openvswitch.org
>> Subject: [ovs-dev] [PATCH] Windows: Fix wmi.c to use count of wchar_t
>> instead of sizeof
>> 
>> wcscat_s and wcscpy_s requires number of elements as argument. wchar_t
>> uses 2 bytes for storage and using sizeof(internal_port_query) causes access
>> violation error on Windows 2012 R2 (64 bit). This patch introduces a
>> count_of() function to return the count of the wchar_t array.
>> 
>> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
>> Reported-by: Sairam Venugopal <vsairam@vmware.com>
>> Reported-at: openvswitch/ovs-issues#121
>> ---
>>  lib/wmi.c | 34 +++++++++++++++++++---------------
>>  1 file changed, 19 insertions(+), 15 deletions(-)
>>
diff mbox

Patch

diff --git a/lib/wmi.c b/lib/wmi.c
index e38b482..a9dfd27 100644
--- a/lib/wmi.c
+++ b/lib/wmi.c
@@ -350,6 +350,8 @@  tranform_wide(char *name, wchar_t *wide_name)
     return true;
 }
 
+#define count_of(array) (sizeof(array)/sizeof(array[0]))
+
 /* This function will delete a switch internal port with a given name as input
  * executing "RemoveResourceSettings" as per documentation:
  * https://msdn.microsoft.com/en-us/library/hh850277%28v=vs.85%29.aspx
@@ -414,9 +416,9 @@  delete_wmi_port(char *name)
         retval = false;
         goto error;
     }
-    wcscat_s(internal_port_query, sizeof(internal_port_query), wide_name);
+    wcscat_s(internal_port_query, count_of(internal_port_query), wide_name);
 
-    wcscat_s(internal_port_query, sizeof(internal_port_query), L"\"");
+    wcscat_s(internal_port_query, count_of(internal_port_query), L"\"");
 
     hres = psvc->lpVtbl->ExecQuery(psvc,
                                    L"WQL",
@@ -626,6 +628,7 @@  error:
     return retval;
 }
 
+
 /* This function will create an internal port on the switch given a given name
  * executing the method AddResourceSettings as per documentation:
  * https://msdn.microsoft.com/en-us/library/hh850019%28v=vs.85%29.aspx.
@@ -700,9 +703,10 @@  create_wmi_port(char *name) {
         retval = false;
         goto error;
     }
-    wcscat_s(internal_port_query, sizeof(internal_port_query), wide_name);
 
-    wcscat_s(internal_port_query, sizeof(internal_port_query), L"\"");
+    wcscat_s(internal_port_query, count_of(internal_port_query), wide_name);
+
+    wcscat_s(internal_port_query, count_of(internal_port_query), L"\"");
     hres = psvc->lpVtbl->ExecQuery(psvc,
                                    L"WQL",
                                    internal_port_query,
@@ -748,7 +752,7 @@  create_wmi_port(char *name) {
         retval = false;
         goto error;
     }
-    wcscpy_s(internal_port_query, sizeof(internal_port_query),
+    wcscpy_s(internal_port_query, count_of(internal_port_query),
              L"SELECT * FROM Msvm_VirtualEthernetSwitch WHERE Name = \"");
 
     hres = pcls_obj->lpVtbl->Get(pcls_obj, L"SystemName", 0,
@@ -758,7 +762,7 @@  create_wmi_port(char *name) {
         goto error;
     }
 
-    wcscat_s(internal_port_query, sizeof(internal_port_query),
+    wcscat_s(internal_port_query, count_of(internal_port_query),
              vt_prop.bstrVal);
 
     VariantClear(&vt_prop);
@@ -780,7 +784,7 @@  create_wmi_port(char *name) {
     }
 
     /* Get the switch object on which the extension is activated. */
-    wcscat_s(internal_port_query, sizeof(internal_port_query), L"\"");
+    wcscat_s(internal_port_query, count_of(internal_port_query), L"\"");
     hres = psvc->lpVtbl->ExecQuery(psvc,
                                    L"WQL",
                                    internal_port_query,
@@ -810,11 +814,11 @@  create_wmi_port(char *name) {
         goto error;
     }
 
-    wcscpy_s(internal_port_query, sizeof(internal_port_query),
+    wcscpy_s(internal_port_query, count_of(internal_port_query),
              L"SELECT * FROM Msvm_VirtualEthernetSwitchSettingData WHERE "
              L"ElementName = \"");
 
-    wcscat_s(internal_port_query, sizeof(internal_port_query),
+    wcscat_s(internal_port_query, count_of(internal_port_query),
              vt_prop.bstrVal);
     VariantClear(&vt_prop);
 
@@ -831,11 +835,11 @@  create_wmi_port(char *name) {
      * Uniquely identifies an instance of this class. This property is
      * inherited from CIM_SettingData and is always
      * set to "Microsoft:GUID\DeviceSpecificData". */
-    wcscat_s(internal_port_query, sizeof(internal_port_query),
+    wcscat_s(internal_port_query, count_of(internal_port_query),
              L"\" AND InstanceID  = \"Microsoft:");
-    wcscat_s(internal_port_query, sizeof(internal_port_query),
+    wcscat_s(internal_port_query, count_of(internal_port_query),
              vt_prop.bstrVal);
-    wcscat_s(internal_port_query, sizeof(internal_port_query),
+    wcscat_s(internal_port_query, count_of(internal_port_query),
              L"\"");
 
     VariantClear(&vt_prop);
@@ -1135,10 +1139,10 @@  create_wmi_port(char *name) {
         goto error;
     }
 
-    wcscpy_s(internal_port_query, sizeof(internal_port_query),
+    wcscpy_s(internal_port_query, count_of(internal_port_query),
              L"SELECT * FROM MSFT_NetAdapter WHERE Name LIKE '%%");
-    wcscat_s(internal_port_query, sizeof(internal_port_query), wide_name);
-    wcscat_s(internal_port_query, sizeof(internal_port_query), L"%%'");
+    wcscat_s(internal_port_query, count_of(internal_port_query), wide_name);
+    wcscat_s(internal_port_query, count_of(internal_port_query), L"%%'");
 
     /* Get the object with the port name equal to name on the CIM. */
     hres = psvc->lpVtbl->ExecQuery(psvc,