Acked-by: Gert Doering <g...@greenie.muc.de> This is not a truly formal windows code review, but I'm taking this in as the resulting builds have been tested by various people and seem to work well, and we have so many patches on top of this already queued that patchwork needs to go to multipage display...
(I *have* glanced over the code to see that there isn't anything that
looks "suspicious" - and of course it does not modify openvpn code, so
there is no risk for introducing remote exploits, crypto breaches, etc.)
I found a few things I'd like to see cleaned up eventually...
+ /* Create TLS data. */
+ struct openvpnmsica_tls_data *s = (struct
openvpnmsica_tls_data*)malloc(sizeof(struct openvpnmsica_tls_data));
+ memset(s, 0, sizeof(struct openvpnmsica_tls_data));
... this could be done with calloc() to allocate-and-clear, but it
certainly needs a NULL ptr check before using "s"...
(As a side note: the use of "TLS" for "thread local storage" is somewhat
confusing when you've been staring at TLS = "transport layer security"
before that)
+ /* Prepare the message record. The record will contain up to four fields. *
/
+ MSIHANDLE hRecordProg = MsiCreateRecord(4);
+
+ {
+ /* Field 2: The message string. */
+ char szBufStack[128];
do we still need extra nesting just to get a "local" variable declaration?
I thought we can have & use C99 everywhere today, which avoids this extra
nesting level (in FindTAPInterfaces(), no extra brackets are needed).
+ /* Create and fill operation struct. */
+ size_t value_size = (_tcslen(value) + 1) * sizeof(TCHAR);
+ struct msica_op_string *op = (struct msica_op_string*)malloc(sizeof(struct
msica_op_string) + value_size);
more malloc()ing with no return value check... unless there are guarantees
that "malloc() smaller <x> bytes will always succeed in MSI context", this
does not look like good practice.
Then, it would be nice if file headers had a short comment "what is this
doing" (like, I just read through "msiex.c", and having an idea what I
can find in there makes life easier).
Does some other patch in the series have an "overall picture" document
that explains how the "large picture" in MSI custom DLL works? Like,
first happens <this>, than <that>, and for a silent installer <there> will
be a change? Maybe just a pointer to a MS document?
Test compiled on ubuntu 16.04/mingw ("so it compiles for mingw") and
on Linux ("so it does not cause any autoconf-induced funkiness elsewhere").
I have test-run the mingw-compilede tapctl on Win10, and "tapctl list"
"did something" (= it did not crash or complain about missing DLLs) - but
it did not actually *work*. The machine has no TAP interfaces, just one
LAN card - and "tapctl list" prints out 2 or 10 different "LAN" adapters
(see attached image). Not sure this is how it should be... after I
installed OpenVPN & had it create a TAP interface, I still have 9 times
"LAN-Verbindung" but I also get an "Ethernet 2" now (which is correct).
Your patch has been applied to the master branch.
commit ce68686f1e2d6f9a78ea7395560af78f81234da2
Author: Simon Rozman
Date: Wed Oct 10 21:23:37 2018 +0200
Introduce tapctl.exe utility and openvpnmsica.dll MSI CA
Acked-by: Gert Doering <g...@greenie.muc.de>
Message-Id: <20181010192337.6984-1-si...@rozman.si>
URL:
https://www.mail-archive.com/search?l=mid&q=20181010192337.6984-1-si...@rozman.si
Signed-off-by: Gert Doering <g...@greenie.muc.de>
--
kind regards,
Gert Doering
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
