[ntp:hackers] Re-adding support of Windows shared memory to NTP
juergen.perlinger at t-online.de
Fri Apr 26 06:03:07 UTC 2019
On 4/25/19 2:22 PM, Jolidon, Jérôme wrote:
> Note: the mail below has already been sent to the list before, but for some unknown reason it was not forwarded. I'm resending it to see if it was a fluke or something else.
> Hello all,
> I have spent a bit of time to add support of shared memory in the Windows builds of ntp. The code resides in a fork on github, at the address https://github.com/jjolidon/ntp. The git patches are also attached.
> I would very much appreciate your thoughts and comments.
> If the code fully suits the quality requirements and coding conventions of ntp, feel free to include it in the baseline. Otherwise, I am prepared to spend some time for corrections if interest is shown.
> The code was already mostly existing but I had to make a few changes to make it safer and fully functional:
> - The code did not set any ACL when the forall flag was set. This could lead to issues with other users grabbing all permissions on the segment, even though it is mitigated by the fact that the segment always required admin privileges for opening (see below). I have changed that, the ACL are now always set.
> - Windows services always open shared memory in the Global namespace, even when the segment name is prefixed with Local, so the distinction between forall and not forall is less evident for the services. I have translated that to ACL mappings: ownership of the segment is always set to the current user, and r/w permission are added for all logged users when the forall flag is set.
Oha... That's definitely a thing to consider.
> - In addition to these ACL changes, I have implemented a whitelisting principle for the ACL: the code reads a registry key and adds any SID found in the key with r/w permissions. This is done whether the forall flag is set or not. Potentially the permissions set through the registry could be redundant with the forall flag, but this does not seem problematic.
That's a good idea, as far as I can tell. As long as the driver runs
*without* the registry key, too.
> In addition to these changes, I have made changes to the build configurations in order to be able to build with Visual Studio 2017 on recent Windows 10 SDKs:
> - I have created a Visual Studio 2017 solution, as was done for the previous versions of the tool.
> - Recent SDKs include a definition of timespec. I have created a conditional compilation symbol that hides the in-house structure when set. It is specifically enabled in the Visual Studio 2017 solution. This should not be problematic to other platforms and toolchains, I believe.
> - During builds Visual Studio mentioned a code quality issue relative to the order of definitions of STATUS_SEVERITY_WARNING (0x2) not respecting the byte order, as it was defined before STATUS_SEVERITY_SUCCESS (0x0) and STATUS_SEVERITY_INFORMATIONAL (0x1). This is a very minor fix but I included it nonetheless.
> - The compilation of the shared memory support is enabled in the Visual Studio 2017 solution.
> Thanks in advance for your attention and your reactions.
So far we did not go beyond VS2015 with the builds. And maintaining all
the various studio versions becomes a real chore.
(I'm still voting for CMAKE, at least for the windows builds. But that's
a different topic.)
It's a pity you did that in the git repo... NTF had to move all its
infrastructure and the forward/backward migration between git and
bitkeeper broke. It's still not working (though I'm told there's
progress) so integrating this with the current STABLE or DEV branch
might be some work.
I'll definitely have a look.
More information about the hackers