| Message ID | 20220814215303.305-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] tun: properly handle device interface list | expand |
Hi, On 14/08/2022 23:53, Lev Stipakov wrote: > From: Lev Stipakov <lev@openvpn.net> > > Device interface is a path which is used by userspace > to access device. A driver can create one or more device > interfaces and specify "reference string", so that userspace > could enumerate all device interfaces in the list and pick > the corrct one which ends with reference string. > > Before our code had an assumption that either driver > creates only one device interface or the "right" interface > is alwways first in the list. As it turned out, that assumtion > does not always hold, so here we iterate through all device > interfaces in the list. > > In follow-up dco-win patch we pick the device interface > from the list which ends with specific reference string. > > Signed-off-by: Lev Stipakov <lev@openvpn.net> [This is actually v3] The commit message does not say anything using gc_malloc() (although it is reported below in te version log). Maybe one line could be added on the fly before commit? :-) The reason for switching to gc_malloc is that this buffer is actually used like a string and not like an actual buffer, so it does not make sense to use the struct buffer at all. I tested this patch with Lev and it works as expected. The windows API does not say anything about the empty string at the end of the list, but having that seems to be the actual behaviour. Acked-by: Antonio Quartulli <a@unstable.cc> > --- > > v3: use gc_malloc() and "char *" instead of buffer and improve commit message > > v2: uncrustify > > > src/openvpn/tun.c | 40 +++++++++++++++++++++++----------------- > src/openvpn/tun.h | 2 +- > 2 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > index 11025267..50193939 100644 > --- a/src/openvpn/tun.c > +++ b/src/openvpn/tun.c > @@ -3722,7 +3722,6 @@ get_device_instance_id_interface(struct gc_arena *gc) > LONG status; > ULONG dev_interface_list_size; > CONFIGRET cr; > - struct buffer dev_interface_list; > > ZeroMemory(&device_info_data, sizeof(SP_DEVINFO_DATA)); > device_info_data.cbSize = sizeof(SP_DEVINFO_DATA); > @@ -3775,9 +3774,9 @@ get_device_instance_id_interface(struct gc_arena *gc) > goto next; > } > > - dev_interface_list = alloc_buf_gc(dev_interface_list_size, gc); > + char *dev_interface_list = gc_malloc(dev_interface_list_size, false, gc); > cr = CM_Get_Device_Interface_List((LPGUID)&GUID_DEVINTERFACE_NET, device_instance_id, > - BSTR(&dev_interface_list), > + dev_interface_list, > dev_interface_list_size, > CM_GET_DEVICE_INTERFACE_LIST_PRESENT); > if (cr != CR_SUCCESS) > @@ -3785,21 +3784,29 @@ get_device_instance_id_interface(struct gc_arena *gc) > goto next; > } > > - struct device_instance_id_interface *dev_if; > - ALLOC_OBJ_CLEAR_GC(dev_if, struct device_instance_id_interface, gc); > - dev_if->net_cfg_instance_id = (unsigned char *)string_alloc((char *)net_cfg_instance_id, gc); > - dev_if->device_interface_list = string_alloc(BSTR(&dev_interface_list), gc); > + char *dev_if = dev_interface_list; > > - /* link into return list */ > - if (!first) > + /* device interface list ends with empty string */ > + while (strlen(dev_if) > 0) > { > - first = dev_if; > - } > - if (last) > - { > - last->next = dev_if; > + struct device_instance_id_interface *dev_iif; > + ALLOC_OBJ_CLEAR_GC(dev_iif, struct device_instance_id_interface, gc); > + dev_iif->net_cfg_instance_id = (unsigned char *)string_alloc((char *)net_cfg_instance_id, gc); > + dev_iif->device_interface = string_alloc(dev_if, gc); > + > + /* link into return list */ > + if (!first) > + { > + first = dev_iif; > + } > + if (last) > + { > + last->next = dev_iif; > + } > + last = dev_iif; > + > + dev_if += strlen(dev_if) + 1; > } > - last = dev_if; > > next: > RegCloseKey(dev_key); > @@ -6475,12 +6482,11 @@ tun_try_open_device(struct tuntap *tt, const char *device_guid, const struct dev > { > const struct device_instance_id_interface *dev_if; > > - /* Open Wintun adapter */ > for (dev_if = device_instance_id_interface; dev_if != NULL; dev_if = dev_if->next) > { > if (strcmp((const char *)dev_if->net_cfg_instance_id, device_guid) == 0) > { > - path = dev_if->device_interface_list; > + path = dev_if->device_interface; > break; > } > } > diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h > index ea4946e9..661e4d01 100644 > --- a/src/openvpn/tun.h > +++ b/src/openvpn/tun.h > @@ -388,7 +388,7 @@ struct panel_reg > struct device_instance_id_interface > { > LPBYTE net_cfg_instance_id; > - const char *device_interface_list; > + const char *device_interface; > struct device_instance_id_interface *next; > }; >
Did a bit of "staring at the code", all looks quite reasonable.
My Ubuntu 18/mingw test rig exploded after commit 2e359a088226a (for
the "make dist" part, it requires libpcap-ng-dev, but apt refuses to
install that...) so I could not compile-test.
Commit message extended with "use gc_malloc()..."
Your patch has been applied to the master branch.
commit 5c4fa12b0be29dcb18edac62c8c29f91f4ff59b1
Author: Lev Stipakov
Date: Sun Aug 14 23:53:03 2022 +0200
tun: properly handle device interface list
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Antonio Quartulli <a@unstable.cc>
Message-Id: <20220814215303.305-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24938.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 11025267..50193939 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -3722,7 +3722,6 @@ get_device_instance_id_interface(struct gc_arena *gc) LONG status; ULONG dev_interface_list_size; CONFIGRET cr; - struct buffer dev_interface_list; ZeroMemory(&device_info_data, sizeof(SP_DEVINFO_DATA)); device_info_data.cbSize = sizeof(SP_DEVINFO_DATA); @@ -3775,9 +3774,9 @@ get_device_instance_id_interface(struct gc_arena *gc) goto next; } - dev_interface_list = alloc_buf_gc(dev_interface_list_size, gc); + char *dev_interface_list = gc_malloc(dev_interface_list_size, false, gc); cr = CM_Get_Device_Interface_List((LPGUID)&GUID_DEVINTERFACE_NET, device_instance_id, - BSTR(&dev_interface_list), + dev_interface_list, dev_interface_list_size, CM_GET_DEVICE_INTERFACE_LIST_PRESENT); if (cr != CR_SUCCESS) @@ -3785,21 +3784,29 @@ get_device_instance_id_interface(struct gc_arena *gc) goto next; } - struct device_instance_id_interface *dev_if; - ALLOC_OBJ_CLEAR_GC(dev_if, struct device_instance_id_interface, gc); - dev_if->net_cfg_instance_id = (unsigned char *)string_alloc((char *)net_cfg_instance_id, gc); - dev_if->device_interface_list = string_alloc(BSTR(&dev_interface_list), gc); + char *dev_if = dev_interface_list; - /* link into return list */ - if (!first) + /* device interface list ends with empty string */ + while (strlen(dev_if) > 0) { - first = dev_if; - } - if (last) - { - last->next = dev_if; + struct device_instance_id_interface *dev_iif; + ALLOC_OBJ_CLEAR_GC(dev_iif, struct device_instance_id_interface, gc); + dev_iif->net_cfg_instance_id = (unsigned char *)string_alloc((char *)net_cfg_instance_id, gc); + dev_iif->device_interface = string_alloc(dev_if, gc); + + /* link into return list */ + if (!first) + { + first = dev_iif; + } + if (last) + { + last->next = dev_iif; + } + last = dev_iif; + + dev_if += strlen(dev_if) + 1; } - last = dev_if; next: RegCloseKey(dev_key); @@ -6475,12 +6482,11 @@ tun_try_open_device(struct tuntap *tt, const char *device_guid, const struct dev { const struct device_instance_id_interface *dev_if; - /* Open Wintun adapter */ for (dev_if = device_instance_id_interface; dev_if != NULL; dev_if = dev_if->next) { if (strcmp((const char *)dev_if->net_cfg_instance_id, device_guid) == 0) { - path = dev_if->device_interface_list; + path = dev_if->device_interface; break; } } diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index ea4946e9..661e4d01 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -388,7 +388,7 @@ struct panel_reg struct device_instance_id_interface { LPBYTE net_cfg_instance_id; - const char *device_interface_list; + const char *device_interface; struct device_instance_id_interface *next; };