[Openvpn-devel] Remove automatic service

Message ID 1616991798-7179-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Remove automatic service | expand

Commit Message

Selva Nair March 28, 2021, 5:23 p.m. UTC
From: Selva Nair <selva.nair@gmail.com>

This has been replaced by openvpnserv2 since 2.4.0 and we have
stopped setting up this service in the installer since 2.5.0.

Get rid of the unused code. The mechanics of supporting multiple
services with the same executable is retained for possible future use.

For backwards compatibility, the command line option -instance
is unchanged as "-instance <name> id" although <name>="interactive"
is the only supported value now.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpnserv/Makefile.am |   1 -
 src/openvpnserv/automatic.c | 434 --------------------------------------------
 src/openvpnserv/service.c   |  46 +++--
 src/openvpnserv/service.h   |   6 -
 4 files changed, 21 insertions(+), 466 deletions(-)
 delete mode 100644 src/openvpnserv/automatic.c

Comments

Lev Stipakov March 31, 2021, 3:34 a.m. UTC | #1
Hi,

Patch looks fine, apart missing change to VS project:

diff --git a/src/openvpnserv/openvpnserv.vcxproj
b/src/openvpnserv/openvpnserv.vcxproj
index 5e973df4..d0f210ad 100644
--- a/src/openvpnserv/openvpnserv.vcxproj
+++ b/src/openvpnserv/openvpnserv.vcxproj
@@ -115,7 +115,6 @@
     </Link>
   </ItemDefinitionGroup>
   <ItemGroup>
-    <ClCompile Include="automatic.c" />
     <ClCompile Include="common.c" />
     <ClCompile Include="interactive.c" />
     <ClCompile Include="service.c" />
diff --git a/src/openvpnserv/openvpnserv.vcxproj.filters
b/src/openvpnserv/openvpnserv.vcxproj.filters
index 41ad3e80..389dac31 100644
--- a/src/openvpnserv/openvpnserv.vcxproj.filters
+++ b/src/openvpnserv/openvpnserv.vcxproj.filters
@@ -18,9 +18,6 @@
     <ClCompile Include="service.c">
       <Filter>Source Files</Filter>
     </ClCompile>
-    <ClCompile Include="automatic.c">
-      <Filter>Source Files</Filter>
-    </ClCompile>
     <ClCompile Include="common.c">
       <Filter>Source Files</Filter>
     </ClCompile>

I checked MSI and NSIS installers in openvpn-build. MSI installer
doesn't touch automatic service so we're good. NSIS installer script,
however, has a code which
installs "OpenVPNServiceLegacy", so we will have to update it.

I built and tested an NSIS installer with that patch and smoke-tested
on Windows 10. If Gert can modify VS project with the above diff,
consider it as an ACK from me.

ma 29. maalisk. 2021 klo 7.24 selva.nair@gmail.com kirjoitti:
>
> From: Selva Nair <selva.nair@gmail.com>
>
> This has been replaced by openvpnserv2 since 2.4.0 and we have
> stopped setting up this service in the installer since 2.5.0.
>
> Get rid of the unused code. The mechanics of supporting multiple
> services with the same executable is retained for possible future use.
>
> For backwards compatibility, the command line option -instance
> is unchanged as "-instance <name> id" although <name>="interactive"
> is the only supported value now.
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
>  src/openvpnserv/Makefile.am |   1 -
>  src/openvpnserv/automatic.c | 434 --------------------------------------------
>  src/openvpnserv/service.c   |  46 +++--
>  src/openvpnserv/service.h   |   6 -
>  4 files changed, 21 insertions(+), 466 deletions(-)
>  delete mode 100644 src/openvpnserv/automatic.c
>
> diff --git a/src/openvpnserv/Makefile.am b/src/openvpnserv/Makefile.am
> index 5dc38c9..3e69125 100644
> --- a/src/openvpnserv/Makefile.am
> +++ b/src/openvpnserv/Makefile.am
> @@ -31,7 +31,6 @@ endif
>
>  openvpnserv_SOURCES = \
>          common.c \
> -       automatic.c \
>         interactive.c \
>         service.c service.h \
>         validate.c validate.h \
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> deleted file mode 100644
> index 0ba222a..0000000
> --- a/src/openvpnserv/automatic.c
> +++ /dev/null
> @@ -1,434 +0,0 @@
> -/*
> - *  OpenVPN -- An application to securely tunnel IP networks
> - *             over a single TCP/UDP port, with support for SSL/TLS-based
> - *             session authentication and key exchange,
> - *             packet encryption, packet authentication, and
> - *             packet compression.
> - *
> - *  Copyright (C) 2002-2018 OpenVPN Inc <sales@openvpn.net>
> - *
> - *  This program is free software; you can redistribute it and/or modify
> - *  it under the terms of the GNU General Public License version 2
> - *  as published by the Free Software Foundation.
> - *
> - *  This program is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *  GNU General Public License for more details.
> - *
> - *  You should have received a copy of the GNU General Public License along
> - *  with this program; if not, write to the Free Software Foundation, Inc.,
> - *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> - */
> -
> -/*
> - * This program allows one or more OpenVPN processes to be started
> - * as a service.  To build, you must get the service sample from the
> - * Platform SDK and replace Simple.c with this file.
> - *
> - * You should also apply service.patch to
> - * service.c and service.h from the Platform SDK service sample.
> - *
> - * This code is designed to be built with the mingw compiler.
> - */
> -
> -#include "service.h"
> -
> -#include <stdio.h>
> -#include <stdarg.h>
> -#include <stdbool.h>
> -#include <process.h>
> -
> -static SERVICE_STATUS_HANDLE service;
> -static SERVICE_STATUS status = { .dwServiceType = SERVICE_WIN32_SHARE_PROCESS };
> -
> -openvpn_service_t automatic_service = {
> -    automatic,
> -    TEXT(PACKAGE_NAME "ServiceLegacy"),
> -    TEXT(PACKAGE_NAME " Legacy Service"),
> -    TEXT(SERVICE_DEPENDENCIES),
> -    SERVICE_DEMAND_START
> -};
> -
> -struct security_attributes
> -{
> -    SECURITY_ATTRIBUTES sa;
> -    SECURITY_DESCRIPTOR sd;
> -};
> -
> -static HANDLE exit_event = NULL;
> -
> -/* clear an object */
> -#define CLEAR(x) memset(&(x), 0, sizeof(x))
> -
> -
> -bool
> -init_security_attributes_allow_all(struct security_attributes *obj)
> -{
> -    CLEAR(*obj);
> -
> -    obj->sa.nLength = sizeof(SECURITY_ATTRIBUTES);
> -    obj->sa.lpSecurityDescriptor = &obj->sd;
> -    obj->sa.bInheritHandle = TRUE;
> -    if (!InitializeSecurityDescriptor(&obj->sd, SECURITY_DESCRIPTOR_REVISION))
> -    {
> -        return false;
> -    }
> -    if (!SetSecurityDescriptorDacl(&obj->sd, TRUE, NULL, FALSE))
> -    {
> -        return false;
> -    }
> -    return true;
> -}
> -
> -HANDLE
> -create_event(LPCTSTR name, bool allow_all, bool initial_state, bool manual_reset)
> -{
> -    if (allow_all)
> -    {
> -        struct security_attributes sa;
> -        if (!init_security_attributes_allow_all(&sa))
> -        {
> -            return NULL;
> -        }
> -        return CreateEvent(&sa.sa, (BOOL)manual_reset, (BOOL)initial_state, name);
> -    }
> -    else
> -    {
> -        return CreateEvent(NULL, (BOOL)manual_reset, (BOOL)initial_state, name);
> -    }
> -}
> -
> -void
> -close_if_open(HANDLE h)
> -{
> -    if (h != NULL)
> -    {
> -        CloseHandle(h);
> -    }
> -}
> -
> -static bool
> -match(const WIN32_FIND_DATA *find, LPCTSTR ext)
> -{
> -    if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
> -    {
> -        return false;
> -    }
> -
> -    if (*ext == TEXT('\0'))
> -    {
> -        return true;
> -    }
> -
> -    /* find the pointer to that last '.' in filename and match ext against the rest */
> -
> -    const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.'));
> -    return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0;
> -}
> -
> -/*
> - * Modify the extension on a filename.
> - */
> -static bool
> -modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
> -{
> -    size_t i;
> -
> -    if (size > 0 && (_tcslen(src) + 1) <= size)
> -    {
> -        _tcscpy_s(dest, size, src);
> -        dest [size - 1] = TEXT('\0');
> -        i = _tcslen(dest);
> -        while (i-- > 0)
> -        {
> -            if (dest[i] == TEXT('\\'))
> -            {
> -                break;
> -            }
> -            if (dest[i] == TEXT('.'))
> -            {
> -                dest[i] = TEXT('\0');
> -                break;
> -            }
> -        }
> -        if (_tcslen(dest) + _tcslen(newext) + 2 <= size)
> -        {
> -            _tcscat_s(dest, size, TEXT("."));
> -            _tcscat_s(dest, size, newext);
> -            return true;
> -        }
> -        dest[0] = TEXT('\0');
> -    }
> -    return false;
> -}
> -
> -static DWORD WINAPI
> -ServiceCtrlAutomatic(DWORD ctrl_code, DWORD event, LPVOID data, LPVOID ctx)
> -{
> -    SERVICE_STATUS *status = ctx;
> -    switch (ctrl_code)
> -    {
> -        case SERVICE_CONTROL_STOP:
> -            status->dwCurrentState = SERVICE_STOP_PENDING;
> -            ReportStatusToSCMgr(service, status);
> -            if (exit_event)
> -            {
> -                SetEvent(exit_event);
> -            }
> -            return NO_ERROR;
> -
> -        case SERVICE_CONTROL_INTERROGATE:
> -            return NO_ERROR;
> -
> -        default:
> -            return ERROR_CALL_NOT_IMPLEMENTED;
> -    }
> -}
> -
> -
> -VOID WINAPI
> -ServiceStartAutomaticOwn(DWORD dwArgc, LPTSTR *lpszArgv)
> -{
> -    status.dwServiceType = SERVICE_WIN32_OWN_PROCESS;
> -    ServiceStartAutomatic(dwArgc, lpszArgv);
> -}
> -
> -
> -VOID WINAPI
> -ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)
> -{
> -    DWORD error = NO_ERROR;
> -    settings_t settings;
> -    TCHAR event_name[256];
> -
> -    service = RegisterServiceCtrlHandlerEx(automatic_service.name, ServiceCtrlAutomatic, &status);
> -    if (!service)
> -    {
> -        return;
> -    }
> -
> -    status.dwCurrentState = SERVICE_START_PENDING;
> -    status.dwServiceSpecificExitCode = NO_ERROR;
> -    status.dwWin32ExitCode = NO_ERROR;
> -    status.dwWaitHint = 3000;
> -
> -    if (!ReportStatusToSCMgr(service, &status))
> -    {
> -        MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr #1 failed"));
> -        goto finish;
> -    }
> -
> -    /*
> -     * Create our exit event
> -     * This event is initially created in the non-signaled
> -     * state.  It will transition to the signaled state when
> -     * we have received a terminate signal from the Service
> -     * Control Manager which will cause an asynchronous call
> -     * of ServiceStop below.
> -     */
> -
> -    openvpn_sntprintf(event_name, _countof(event_name), TEXT(PACKAGE "%s_exit_1"), service_instance);
> -    exit_event = create_event(event_name, false, false, true);
> -    if (!exit_event)
> -    {
> -        MsgToEventLog(M_ERR, TEXT("CreateEvent failed"));
> -        goto finish;
> -    }
> -
> -    /*
> -     * If exit event is already signaled, it means we were not
> -     * shut down properly.
> -     */
> -    if (WaitForSingleObject(exit_event, 0) != WAIT_TIMEOUT)
> -    {
> -        MsgToEventLog(M_ERR, TEXT("Exit event is already signaled -- we were not shut down properly"));
> -        goto finish;
> -    }
> -
> -    if (!ReportStatusToSCMgr(service, &status))
> -    {
> -        MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr #2 failed"));
> -        goto finish;
> -    }
> -
> -    /*
> -     * Read info from registry in key HKLM\SOFTWARE\OpenVPN
> -     */
> -    error = GetOpenvpnSettings(&settings);
> -    if (error != ERROR_SUCCESS)
> -    {
> -        goto finish;
> -    }
> -
> -    /*
> -     * Instantiate an OpenVPN process for each configuration
> -     * file found.
> -     */
> -    {
> -        WIN32_FIND_DATA find_obj;
> -        HANDLE find_handle;
> -        BOOL more_files;
> -        TCHAR find_string[MAX_PATH];
> -
> -        openvpn_sntprintf(find_string, _countof(find_string), TEXT("%s\\*"), settings.config_dir);
> -
> -        find_handle = FindFirstFile(find_string, &find_obj);
> -        if (find_handle == INVALID_HANDLE_VALUE)
> -        {
> -            MsgToEventLog(M_ERR, TEXT("Cannot get configuration file list using: %s"), find_string);
> -            goto finish;
> -        }
> -
> -        /*
> -         * Loop over each config file
> -         */
> -        do
> -        {
> -            HANDLE log_handle = NULL;
> -            STARTUPINFO start_info;
> -            PROCESS_INFORMATION proc_info;
> -            struct security_attributes sa;
> -            TCHAR log_file[MAX_PATH];
> -            TCHAR log_path[MAX_PATH];
> -            TCHAR command_line[256];
> -
> -            CLEAR(start_info);
> -            CLEAR(proc_info);
> -            CLEAR(sa);
> -
> -            if (!ReportStatusToSCMgr(service, &status))
> -            {
> -                MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr #3 failed"));
> -                FindClose(find_handle);
> -                goto finish;
> -            }
> -
> -            /* does file have the correct type and extension? */
> -            if (match(&find_obj, settings.ext_string))
> -            {
> -                /* get log file pathname */
> -                if (!modext(log_file, _countof(log_file), find_obj.cFileName, TEXT("log")))
> -                {
> -                    MsgToEventLog(M_ERR, TEXT("Cannot construct logfile name based on: %s"), find_obj.cFileName);
> -                    FindClose(find_handle);
> -                    goto finish;
> -                }
> -                openvpn_sntprintf(log_path, _countof(log_path),
> -                                  TEXT("%s\\%s"), settings.log_dir, log_file);
> -
> -                /* construct command line */
> -                openvpn_sntprintf(command_line, _countof(command_line), TEXT("openvpn --service \"" PACKAGE "%s_exit_1\" 1 --config \"%s\""),
> -                                  service_instance,
> -                                  find_obj.cFileName);
> -
> -                /* Make security attributes struct for logfile handle so it can
> -                 * be inherited. */
> -                if (!init_security_attributes_allow_all(&sa))
> -                {
> -                    error = MsgToEventLog(M_SYSERR, TEXT("InitializeSecurityDescriptor start_" PACKAGE " failed"));
> -                    goto finish;
> -                }
> -
> -                /* open logfile as stdout/stderr for soon-to-be-spawned subprocess */
> -                log_handle = CreateFile(log_path,
> -                                        GENERIC_WRITE,
> -                                        FILE_SHARE_READ,
> -                                        &sa.sa,
> -                                        settings.append ? OPEN_ALWAYS : CREATE_ALWAYS,
> -                                        FILE_ATTRIBUTE_NORMAL,
> -                                        NULL);
> -
> -                if (log_handle == INVALID_HANDLE_VALUE)
> -                {
> -                    error = MsgToEventLog(M_SYSERR, TEXT("Cannot open logfile: %s"), log_path);
> -                    FindClose(find_handle);
> -                    goto finish;
> -                }
> -
> -                /* append to logfile? */
> -                if (settings.append)
> -                {
> -                    if (SetFilePointer(log_handle, 0, NULL, FILE_END) == INVALID_SET_FILE_POINTER)
> -                    {
> -                        error = MsgToEventLog(M_SYSERR, TEXT("Cannot seek to end of logfile: %s"), log_path);
> -                        FindClose(find_handle);
> -                        goto finish;
> -                    }
> -                }
> -
> -                /* fill in STARTUPINFO struct */
> -                GetStartupInfo(&start_info);
> -                start_info.cb = sizeof(start_info);
> -                start_info.dwFlags = STARTF_USESTDHANDLES|STARTF_USESHOWWINDOW;
> -                start_info.wShowWindow = SW_HIDE;
> -                start_info.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
> -                start_info.hStdOutput = start_info.hStdError = log_handle;
> -
> -                /* create an OpenVPN process for one config file */
> -                if (!CreateProcess(settings.exe_path,
> -                                   command_line,
> -                                   NULL,
> -                                   NULL,
> -                                   TRUE,
> -                                   settings.priority | CREATE_NEW_CONSOLE,
> -                                   NULL,
> -                                   settings.config_dir,
> -                                   &start_info,
> -                                   &proc_info))
> -                {
> -                    error = MsgToEventLog(M_SYSERR, TEXT("CreateProcess failed, exe='%s' cmdline='%s' dir='%s'"),
> -                                          settings.exe_path,
> -                                          command_line,
> -                                          settings.config_dir);
> -
> -                    FindClose(find_handle);
> -                    CloseHandle(log_handle);
> -                    goto finish;
> -                }
> -
> -                /* close unneeded handles */
> -                Sleep(1000); /* try to prevent race if we close logfile
> -                              * handle before child process DUPs it */
> -                if (!CloseHandle(proc_info.hProcess)
> -                    || !CloseHandle(proc_info.hThread)
> -                    || !CloseHandle(log_handle))
> -                {
> -                    error = MsgToEventLog(M_SYSERR, TEXT("CloseHandle failed"));
> -                    goto finish;
> -                }
> -            }
> -
> -            /* more files to process? */
> -            more_files = FindNextFile(find_handle, &find_obj);
> -
> -        } while (more_files);
> -
> -        FindClose(find_handle);
> -    }
> -
> -    /* we are now fully started */
> -    status.dwCurrentState = SERVICE_RUNNING;
> -    status.dwWaitHint = 0;
> -    if (!ReportStatusToSCMgr(service, &status))
> -    {
> -        MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr SERVICE_RUNNING failed"));
> -        goto finish;
> -    }
> -
> -    /* wait for our shutdown signal */
> -    if (WaitForSingleObject(exit_event, INFINITE) != WAIT_OBJECT_0)
> -    {
> -        MsgToEventLog(M_ERR, TEXT("wait for shutdown signal failed"));
> -    }
> -
> -finish:
> -    if (exit_event)
> -    {
> -        CloseHandle(exit_event);
> -    }
> -
> -    status.dwCurrentState = SERVICE_STOPPED;
> -    status.dwWin32ExitCode = error;
> -    ReportStatusToSCMgr(service, &status);
> -}
> diff --git a/src/openvpnserv/service.c b/src/openvpnserv/service.c
> index 8101f83..9566f99 100644
> --- a/src/openvpnserv/service.c
> +++ b/src/openvpnserv/service.c
> @@ -224,21 +224,14 @@ int
>  _tmain(int argc, TCHAR *argv[])
>  {
>      /*
> -     * Automatic + Interactive service (as a SERVICE_WIN32_SHARE_PROCESS)
> +     * Interactive service (as a SERVICE_WIN32_SHARE_PROCESS)
>       * This is the default.
>       */
>      const SERVICE_TABLE_ENTRY dispatchTable_shared[] = {
> -        { automatic_service.name, ServiceStartAutomatic },
>          { interactive_service.name, ServiceStartInteractive },
>          { NULL, NULL }
>      };
>
> -    /* Automatic service only (as a SERVICE_WIN32_OWN_PROCESS) */
> -    const SERVICE_TABLE_ENTRY dispatchTable_automatic[] = {
> -        { TEXT(""), ServiceStartAutomaticOwn },
> -        { NULL, NULL }
> -    };
> -
>      /* Interactive service only (as a SERVICE_WIN32_OWN_PROCESS) */
>      const SERVICE_TABLE_ENTRY dispatchTable_interactive[] = {
>          { TEXT(""), ServiceStartInteractiveOwn },
> @@ -247,8 +240,7 @@ _tmain(int argc, TCHAR *argv[])
>
>      const SERVICE_TABLE_ENTRY *dispatchTable = dispatchTable_shared;
>
> -    openvpn_service[0] = automatic_service;
> -    openvpn_service[1] = interactive_service;
> +    openvpn_service[interactive] = interactive_service;
>
>      for (int i = 1; i < argc; i++)
>      {
> @@ -264,29 +256,33 @@ _tmain(int argc, TCHAR *argv[])
>              }
>              else if (_tcsicmp(TEXT("start"), argv[i] + 1) == 0)
>              {
> -                BOOL is_auto = argc < i + 2 || _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0;
> -                return CmdStartService(is_auto ? automatic : interactive);
> +                return CmdStartService(interactive);
>              }
>              else if (argc > i + 2 && _tcsicmp(TEXT("instance"), argv[i] + 1) == 0)
>              {
> -                dispatchTable = _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0 ?
> -                                dispatchTable_automatic :
> -                                dispatchTable_interactive;
> -
> -                service_instance = argv[i + 2];
> -                i += 2;
> +                if (_tcsicmp(TEXT("interactive"), argv[i+1]) == 0)
> +                {
> +                    dispatchTable = dispatchTable_interactive;
> +                    service_instance = argv[i + 2];
> +                    i += 2;
> +                }
> +                else
> +                {
> +                    MsgToEventLog(M_ERR, L"Invalid argument to -instance <%s>. Service not started.", argv[i+1]);
> +                    return 1;
> +                }
>              }
>              else
>              {
> -                _tprintf(TEXT("%s -install        to install the services\n"), APPNAME);
> -                _tprintf(TEXT("%s -start <name>   to start a service (\"automatic\" or \"interactive\")\n"), APPNAME);
> -                _tprintf(TEXT("%s -remove         to remove the services\n"), APPNAME);
> +                _tprintf(TEXT("%s -install        to install the interactive service\n"), APPNAME);
> +                _tprintf(TEXT("%s -start [name]   to start the service (name = \"interactive\" is optional)\n"), APPNAME);
> +                _tprintf(TEXT("%s -remove         to remove the service\n"), APPNAME);
>
>                  _tprintf(TEXT("\nService run-time parameters:\n"));
> -                _tprintf(TEXT("-instance <name> <id>\n")
> -                         TEXT("   Runs the service as an alternate instance. <name> can be \"automatic\" or\n")
> -                         TEXT("   \"interactive\". The service settings will be loaded from\n")
> -                         TEXT("   HKLM\\Software\\" PACKAGE_NAME "<id> registry key, and the interactive service will accept\n")
> +                _tprintf(TEXT("-instance interactive <id>\n")
> +                         TEXT("   Runs the service as an alternate instance.\n")
> +                         TEXT("   The service settings will be loaded from\n")
> +                         TEXT("   HKLM\\Software\\" PACKAGE_NAME "<id> registry key, and the service will accept\n")
>                           TEXT("   requests on \\\\.\\pipe\\" PACKAGE "<id>\\service named pipe.\n"));
>
>                  return 0;
> diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h
> index f5afe2f..32a721f 100644
> --- a/src/openvpnserv/service.h
> +++ b/src/openvpnserv/service.h
> @@ -48,7 +48,6 @@
>  #define M_ERR     (MSG_FLAGS_ERROR)                    /* error */
>
>  typedef enum {
> -    automatic,
>      interactive,
>      _service_max
>  } openvpn_service_type;
> @@ -72,14 +71,9 @@ typedef struct {
>      BOOL append;
>  } settings_t;
>
> -extern openvpn_service_t automatic_service;
>  extern openvpn_service_t interactive_service;
>  extern LPCTSTR service_instance;
>
> -VOID WINAPI ServiceStartAutomaticOwn(DWORD argc, LPTSTR *argv);
> -
> -VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR *argv);
> -
>  VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv);
>
>  VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv);
> --
> 2.1.4
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering March 31, 2021, 4:22 a.m. UTC | #2
Your patch has been applied to the master branch.

I have stared at it for a bit (looks good) and test-compiled it on
Ubuntu 18 / MinGW (succeeded).  I have not tested it (thanks Lev).

I have added the vcxproj diff from Lev's mail, because that is
"sort of obvious" if a source file goes away completely.  We just 
forget all the time :-)

commit 2d5c437f7cf5b4f7d43c297373827d2c3597e955
Author: Selva Nair
Date:   Mon Mar 29 00:23:18 2021 -0400

     Remove automatic service

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <1616991798-7179-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21890.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpnserv/Makefile.am b/src/openvpnserv/Makefile.am
index 5dc38c9..3e69125 100644
--- a/src/openvpnserv/Makefile.am
+++ b/src/openvpnserv/Makefile.am
@@ -31,7 +31,6 @@  endif
 
 openvpnserv_SOURCES = \
         common.c \
-	automatic.c \
 	interactive.c \
 	service.c service.h \
 	validate.c validate.h \
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
deleted file mode 100644
index 0ba222a..0000000
--- a/src/openvpnserv/automatic.c
+++ /dev/null
@@ -1,434 +0,0 @@ 
-/*
- *  OpenVPN -- An application to securely tunnel IP networks
- *             over a single TCP/UDP port, with support for SSL/TLS-based
- *             session authentication and key exchange,
- *             packet encryption, packet authentication, and
- *             packet compression.
- *
- *  Copyright (C) 2002-2018 OpenVPN Inc <sales@openvpn.net>
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License version 2
- *  as published by the Free Software Foundation.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License along
- *  with this program; if not, write to the Free Software Foundation, Inc.,
- *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-/*
- * This program allows one or more OpenVPN processes to be started
- * as a service.  To build, you must get the service sample from the
- * Platform SDK and replace Simple.c with this file.
- *
- * You should also apply service.patch to
- * service.c and service.h from the Platform SDK service sample.
- *
- * This code is designed to be built with the mingw compiler.
- */
-
-#include "service.h"
-
-#include <stdio.h>
-#include <stdarg.h>
-#include <stdbool.h>
-#include <process.h>
-
-static SERVICE_STATUS_HANDLE service;
-static SERVICE_STATUS status = { .dwServiceType = SERVICE_WIN32_SHARE_PROCESS };
-
-openvpn_service_t automatic_service = {
-    automatic,
-    TEXT(PACKAGE_NAME "ServiceLegacy"),
-    TEXT(PACKAGE_NAME " Legacy Service"),
-    TEXT(SERVICE_DEPENDENCIES),
-    SERVICE_DEMAND_START
-};
-
-struct security_attributes
-{
-    SECURITY_ATTRIBUTES sa;
-    SECURITY_DESCRIPTOR sd;
-};
-
-static HANDLE exit_event = NULL;
-
-/* clear an object */
-#define CLEAR(x) memset(&(x), 0, sizeof(x))
-
-
-bool
-init_security_attributes_allow_all(struct security_attributes *obj)
-{
-    CLEAR(*obj);
-
-    obj->sa.nLength = sizeof(SECURITY_ATTRIBUTES);
-    obj->sa.lpSecurityDescriptor = &obj->sd;
-    obj->sa.bInheritHandle = TRUE;
-    if (!InitializeSecurityDescriptor(&obj->sd, SECURITY_DESCRIPTOR_REVISION))
-    {
-        return false;
-    }
-    if (!SetSecurityDescriptorDacl(&obj->sd, TRUE, NULL, FALSE))
-    {
-        return false;
-    }
-    return true;
-}
-
-HANDLE
-create_event(LPCTSTR name, bool allow_all, bool initial_state, bool manual_reset)
-{
-    if (allow_all)
-    {
-        struct security_attributes sa;
-        if (!init_security_attributes_allow_all(&sa))
-        {
-            return NULL;
-        }
-        return CreateEvent(&sa.sa, (BOOL)manual_reset, (BOOL)initial_state, name);
-    }
-    else
-    {
-        return CreateEvent(NULL, (BOOL)manual_reset, (BOOL)initial_state, name);
-    }
-}
-
-void
-close_if_open(HANDLE h)
-{
-    if (h != NULL)
-    {
-        CloseHandle(h);
-    }
-}
-
-static bool
-match(const WIN32_FIND_DATA *find, LPCTSTR ext)
-{
-    if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
-    {
-        return false;
-    }
-
-    if (*ext == TEXT('\0'))
-    {
-        return true;
-    }
-
-    /* find the pointer to that last '.' in filename and match ext against the rest */
-
-    const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.'));
-    return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0;
-}
-
-/*
- * Modify the extension on a filename.
- */
-static bool
-modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
-{
-    size_t i;
-
-    if (size > 0 && (_tcslen(src) + 1) <= size)
-    {
-        _tcscpy_s(dest, size, src);
-        dest [size - 1] = TEXT('\0');
-        i = _tcslen(dest);
-        while (i-- > 0)
-        {
-            if (dest[i] == TEXT('\\'))
-            {
-                break;
-            }
-            if (dest[i] == TEXT('.'))
-            {
-                dest[i] = TEXT('\0');
-                break;
-            }
-        }
-        if (_tcslen(dest) + _tcslen(newext) + 2 <= size)
-        {
-            _tcscat_s(dest, size, TEXT("."));
-            _tcscat_s(dest, size, newext);
-            return true;
-        }
-        dest[0] = TEXT('\0');
-    }
-    return false;
-}
-
-static DWORD WINAPI
-ServiceCtrlAutomatic(DWORD ctrl_code, DWORD event, LPVOID data, LPVOID ctx)
-{
-    SERVICE_STATUS *status = ctx;
-    switch (ctrl_code)
-    {
-        case SERVICE_CONTROL_STOP:
-            status->dwCurrentState = SERVICE_STOP_PENDING;
-            ReportStatusToSCMgr(service, status);
-            if (exit_event)
-            {
-                SetEvent(exit_event);
-            }
-            return NO_ERROR;
-
-        case SERVICE_CONTROL_INTERROGATE:
-            return NO_ERROR;
-
-        default:
-            return ERROR_CALL_NOT_IMPLEMENTED;
-    }
-}
-
-
-VOID WINAPI
-ServiceStartAutomaticOwn(DWORD dwArgc, LPTSTR *lpszArgv)
-{
-    status.dwServiceType = SERVICE_WIN32_OWN_PROCESS;
-    ServiceStartAutomatic(dwArgc, lpszArgv);
-}
-
-
-VOID WINAPI
-ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)
-{
-    DWORD error = NO_ERROR;
-    settings_t settings;
-    TCHAR event_name[256];
-
-    service = RegisterServiceCtrlHandlerEx(automatic_service.name, ServiceCtrlAutomatic, &status);
-    if (!service)
-    {
-        return;
-    }
-
-    status.dwCurrentState = SERVICE_START_PENDING;
-    status.dwServiceSpecificExitCode = NO_ERROR;
-    status.dwWin32ExitCode = NO_ERROR;
-    status.dwWaitHint = 3000;
-
-    if (!ReportStatusToSCMgr(service, &status))
-    {
-        MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr #1 failed"));
-        goto finish;
-    }
-
-    /*
-     * Create our exit event
-     * This event is initially created in the non-signaled
-     * state.  It will transition to the signaled state when
-     * we have received a terminate signal from the Service
-     * Control Manager which will cause an asynchronous call
-     * of ServiceStop below.
-     */
-
-    openvpn_sntprintf(event_name, _countof(event_name), TEXT(PACKAGE "%s_exit_1"), service_instance);
-    exit_event = create_event(event_name, false, false, true);
-    if (!exit_event)
-    {
-        MsgToEventLog(M_ERR, TEXT("CreateEvent failed"));
-        goto finish;
-    }
-
-    /*
-     * If exit event is already signaled, it means we were not
-     * shut down properly.
-     */
-    if (WaitForSingleObject(exit_event, 0) != WAIT_TIMEOUT)
-    {
-        MsgToEventLog(M_ERR, TEXT("Exit event is already signaled -- we were not shut down properly"));
-        goto finish;
-    }
-
-    if (!ReportStatusToSCMgr(service, &status))
-    {
-        MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr #2 failed"));
-        goto finish;
-    }
-
-    /*
-     * Read info from registry in key HKLM\SOFTWARE\OpenVPN
-     */
-    error = GetOpenvpnSettings(&settings);
-    if (error != ERROR_SUCCESS)
-    {
-        goto finish;
-    }
-
-    /*
-     * Instantiate an OpenVPN process for each configuration
-     * file found.
-     */
-    {
-        WIN32_FIND_DATA find_obj;
-        HANDLE find_handle;
-        BOOL more_files;
-        TCHAR find_string[MAX_PATH];
-
-        openvpn_sntprintf(find_string, _countof(find_string), TEXT("%s\\*"), settings.config_dir);
-
-        find_handle = FindFirstFile(find_string, &find_obj);
-        if (find_handle == INVALID_HANDLE_VALUE)
-        {
-            MsgToEventLog(M_ERR, TEXT("Cannot get configuration file list using: %s"), find_string);
-            goto finish;
-        }
-
-        /*
-         * Loop over each config file
-         */
-        do
-        {
-            HANDLE log_handle = NULL;
-            STARTUPINFO start_info;
-            PROCESS_INFORMATION proc_info;
-            struct security_attributes sa;
-            TCHAR log_file[MAX_PATH];
-            TCHAR log_path[MAX_PATH];
-            TCHAR command_line[256];
-
-            CLEAR(start_info);
-            CLEAR(proc_info);
-            CLEAR(sa);
-
-            if (!ReportStatusToSCMgr(service, &status))
-            {
-                MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr #3 failed"));
-                FindClose(find_handle);
-                goto finish;
-            }
-
-            /* does file have the correct type and extension? */
-            if (match(&find_obj, settings.ext_string))
-            {
-                /* get log file pathname */
-                if (!modext(log_file, _countof(log_file), find_obj.cFileName, TEXT("log")))
-                {
-                    MsgToEventLog(M_ERR, TEXT("Cannot construct logfile name based on: %s"), find_obj.cFileName);
-                    FindClose(find_handle);
-                    goto finish;
-                }
-                openvpn_sntprintf(log_path, _countof(log_path),
-                                  TEXT("%s\\%s"), settings.log_dir, log_file);
-
-                /* construct command line */
-                openvpn_sntprintf(command_line, _countof(command_line), TEXT("openvpn --service \"" PACKAGE "%s_exit_1\" 1 --config \"%s\""),
-                                  service_instance,
-                                  find_obj.cFileName);
-
-                /* Make security attributes struct for logfile handle so it can
-                 * be inherited. */
-                if (!init_security_attributes_allow_all(&sa))
-                {
-                    error = MsgToEventLog(M_SYSERR, TEXT("InitializeSecurityDescriptor start_" PACKAGE " failed"));
-                    goto finish;
-                }
-
-                /* open logfile as stdout/stderr for soon-to-be-spawned subprocess */
-                log_handle = CreateFile(log_path,
-                                        GENERIC_WRITE,
-                                        FILE_SHARE_READ,
-                                        &sa.sa,
-                                        settings.append ? OPEN_ALWAYS : CREATE_ALWAYS,
-                                        FILE_ATTRIBUTE_NORMAL,
-                                        NULL);
-
-                if (log_handle == INVALID_HANDLE_VALUE)
-                {
-                    error = MsgToEventLog(M_SYSERR, TEXT("Cannot open logfile: %s"), log_path);
-                    FindClose(find_handle);
-                    goto finish;
-                }
-
-                /* append to logfile? */
-                if (settings.append)
-                {
-                    if (SetFilePointer(log_handle, 0, NULL, FILE_END) == INVALID_SET_FILE_POINTER)
-                    {
-                        error = MsgToEventLog(M_SYSERR, TEXT("Cannot seek to end of logfile: %s"), log_path);
-                        FindClose(find_handle);
-                        goto finish;
-                    }
-                }
-
-                /* fill in STARTUPINFO struct */
-                GetStartupInfo(&start_info);
-                start_info.cb = sizeof(start_info);
-                start_info.dwFlags = STARTF_USESTDHANDLES|STARTF_USESHOWWINDOW;
-                start_info.wShowWindow = SW_HIDE;
-                start_info.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
-                start_info.hStdOutput = start_info.hStdError = log_handle;
-
-                /* create an OpenVPN process for one config file */
-                if (!CreateProcess(settings.exe_path,
-                                   command_line,
-                                   NULL,
-                                   NULL,
-                                   TRUE,
-                                   settings.priority | CREATE_NEW_CONSOLE,
-                                   NULL,
-                                   settings.config_dir,
-                                   &start_info,
-                                   &proc_info))
-                {
-                    error = MsgToEventLog(M_SYSERR, TEXT("CreateProcess failed, exe='%s' cmdline='%s' dir='%s'"),
-                                          settings.exe_path,
-                                          command_line,
-                                          settings.config_dir);
-
-                    FindClose(find_handle);
-                    CloseHandle(log_handle);
-                    goto finish;
-                }
-
-                /* close unneeded handles */
-                Sleep(1000); /* try to prevent race if we close logfile
-                              * handle before child process DUPs it */
-                if (!CloseHandle(proc_info.hProcess)
-                    || !CloseHandle(proc_info.hThread)
-                    || !CloseHandle(log_handle))
-                {
-                    error = MsgToEventLog(M_SYSERR, TEXT("CloseHandle failed"));
-                    goto finish;
-                }
-            }
-
-            /* more files to process? */
-            more_files = FindNextFile(find_handle, &find_obj);
-
-        } while (more_files);
-
-        FindClose(find_handle);
-    }
-
-    /* we are now fully started */
-    status.dwCurrentState = SERVICE_RUNNING;
-    status.dwWaitHint = 0;
-    if (!ReportStatusToSCMgr(service, &status))
-    {
-        MsgToEventLog(M_ERR, TEXT("ReportStatusToSCMgr SERVICE_RUNNING failed"));
-        goto finish;
-    }
-
-    /* wait for our shutdown signal */
-    if (WaitForSingleObject(exit_event, INFINITE) != WAIT_OBJECT_0)
-    {
-        MsgToEventLog(M_ERR, TEXT("wait for shutdown signal failed"));
-    }
-
-finish:
-    if (exit_event)
-    {
-        CloseHandle(exit_event);
-    }
-
-    status.dwCurrentState = SERVICE_STOPPED;
-    status.dwWin32ExitCode = error;
-    ReportStatusToSCMgr(service, &status);
-}
diff --git a/src/openvpnserv/service.c b/src/openvpnserv/service.c
index 8101f83..9566f99 100644
--- a/src/openvpnserv/service.c
+++ b/src/openvpnserv/service.c
@@ -224,21 +224,14 @@  int
 _tmain(int argc, TCHAR *argv[])
 {
     /*
-     * Automatic + Interactive service (as a SERVICE_WIN32_SHARE_PROCESS)
+     * Interactive service (as a SERVICE_WIN32_SHARE_PROCESS)
      * This is the default.
      */
     const SERVICE_TABLE_ENTRY dispatchTable_shared[] = {
-        { automatic_service.name, ServiceStartAutomatic },
         { interactive_service.name, ServiceStartInteractive },
         { NULL, NULL }
     };
 
-    /* Automatic service only (as a SERVICE_WIN32_OWN_PROCESS) */
-    const SERVICE_TABLE_ENTRY dispatchTable_automatic[] = {
-        { TEXT(""), ServiceStartAutomaticOwn },
-        { NULL, NULL }
-    };
-
     /* Interactive service only (as a SERVICE_WIN32_OWN_PROCESS) */
     const SERVICE_TABLE_ENTRY dispatchTable_interactive[] = {
         { TEXT(""), ServiceStartInteractiveOwn },
@@ -247,8 +240,7 @@  _tmain(int argc, TCHAR *argv[])
 
     const SERVICE_TABLE_ENTRY *dispatchTable = dispatchTable_shared;
 
-    openvpn_service[0] = automatic_service;
-    openvpn_service[1] = interactive_service;
+    openvpn_service[interactive] = interactive_service;
 
     for (int i = 1; i < argc; i++)
     {
@@ -264,29 +256,33 @@  _tmain(int argc, TCHAR *argv[])
             }
             else if (_tcsicmp(TEXT("start"), argv[i] + 1) == 0)
             {
-                BOOL is_auto = argc < i + 2 || _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0;
-                return CmdStartService(is_auto ? automatic : interactive);
+                return CmdStartService(interactive);
             }
             else if (argc > i + 2 && _tcsicmp(TEXT("instance"), argv[i] + 1) == 0)
             {
-                dispatchTable = _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0 ?
-                                dispatchTable_automatic :
-                                dispatchTable_interactive;
-
-                service_instance = argv[i + 2];
-                i += 2;
+                if (_tcsicmp(TEXT("interactive"), argv[i+1]) == 0)
+                {
+                    dispatchTable = dispatchTable_interactive;
+                    service_instance = argv[i + 2];
+                    i += 2;
+                }
+                else
+                {
+                    MsgToEventLog(M_ERR, L"Invalid argument to -instance <%s>. Service not started.", argv[i+1]);
+                    return 1;
+                }
             }
             else
             {
-                _tprintf(TEXT("%s -install        to install the services\n"), APPNAME);
-                _tprintf(TEXT("%s -start <name>   to start a service (\"automatic\" or \"interactive\")\n"), APPNAME);
-                _tprintf(TEXT("%s -remove         to remove the services\n"), APPNAME);
+                _tprintf(TEXT("%s -install        to install the interactive service\n"), APPNAME);
+                _tprintf(TEXT("%s -start [name]   to start the service (name = \"interactive\" is optional)\n"), APPNAME);
+                _tprintf(TEXT("%s -remove         to remove the service\n"), APPNAME);
 
                 _tprintf(TEXT("\nService run-time parameters:\n"));
-                _tprintf(TEXT("-instance <name> <id>\n")
-                         TEXT("   Runs the service as an alternate instance. <name> can be \"automatic\" or\n")
-                         TEXT("   \"interactive\". The service settings will be loaded from\n")
-                         TEXT("   HKLM\\Software\\" PACKAGE_NAME "<id> registry key, and the interactive service will accept\n")
+                _tprintf(TEXT("-instance interactive <id>\n")
+                         TEXT("   Runs the service as an alternate instance.\n")
+                         TEXT("   The service settings will be loaded from\n")
+                         TEXT("   HKLM\\Software\\" PACKAGE_NAME "<id> registry key, and the service will accept\n")
                          TEXT("   requests on \\\\.\\pipe\\" PACKAGE "<id>\\service named pipe.\n"));
 
                 return 0;
diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h
index f5afe2f..32a721f 100644
--- a/src/openvpnserv/service.h
+++ b/src/openvpnserv/service.h
@@ -48,7 +48,6 @@ 
 #define M_ERR     (MSG_FLAGS_ERROR)                    /* error */
 
 typedef enum {
-    automatic,
     interactive,
     _service_max
 } openvpn_service_type;
@@ -72,14 +71,9 @@  typedef struct {
     BOOL append;
 } settings_t;
 
-extern openvpn_service_t automatic_service;
 extern openvpn_service_t interactive_service;
 extern LPCTSTR service_instance;
 
-VOID WINAPI ServiceStartAutomaticOwn(DWORD argc, LPTSTR *argv);
-
-VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR *argv);
-
 VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv);
 
 VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv);