[PATCH] validate -n parameter value

2014-06-14 Thread Amos Jeffries 
Update the service_name global to an SBuf and use Tokenizer to validate
the -n argument is a pure alphanumeric.

Amos
=== modified file 'src/WinSvc.cc'
--- src/WinSvc.cc   2014-01-24 01:57:15 +
+++ src/WinSvc.cc   2014-06-14 14:20:56 +
@@ -493,41 +493,41 @@
 
 #if defined(_MSC_VER) /* Microsoft C Compiler ONLY */
 
 newHandler = Squid_Win32InvalidParameterHandler;
 
 oldHandler = _set_invalid_parameter_handler(newHandler);
 
 _CrtSetReportMode(_CRT_ASSERT, 0);
 
 #endif
 #if USE_WIN32_SERVICE
 
 if (WIN32_run_mode == _WIN_SQUID_RUN_MODE_SERVICE) {
 char path[512];
 HKEY hndKey;
 
 if (signal(SIGABRT, WIN32_Abort) == SIG_ERR)
 return 1;
 
 /* Register the service Handler function */
-svcHandle = RegisterServiceCtrlHandler(service_name, WIN32_svcHandler);
+svcHandle = RegisterServiceCtrlHandler(service_name.c_str(), 
WIN32_svcHandler);
 
 if (svcHandle == 0)
 return 1;
 
 /* Set Process work dir to directory cointaining squid.exe */
 GetModuleFileName(NULL, path, 512);
 
 WIN32_module_name=xstrdup(path);
 
 path[strlen(path) - 10] = '\0';
 
 if (SetCurrentDirectory(path) == 0)
 return 1;
 
 safe_free(ConfigFile);
 
 /* get config file from Windows Registry */
 if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, REGKEY, 0, KEY_QUERY_VALUE, 
&hndKey) == ERROR_SUCCESS) {
 DWORD Type = 0;
 DWORD Size = 0;
@@ -654,192 +654,195 @@
 status = GetLastError();
 debugs(1, DBG_IMPORTANT, "SetServiceStatus error " << status);
 }
 
 debugs(1, DBG_IMPORTANT, "Leaving Squid service");
 break;
 
 default:
 debugs(1, DBG_IMPORTANT, "Unrecognized opcode " << Opcode);
 }
 
 return;
 }
 
 void
 WIN32_RemoveService()
 {
 SC_HANDLE schService;
 SC_HANDLE schSCManager;
 
-if (!service_name)
-service_name = xstrdup(APP_SHORTNAME);
+if (service_name.isEmpty())
+service_name = SBuf(APP_SHORTNAME);
 
-strcat(REGKEY, service_name);
+const char *service =  service_name.c_str();
+strcat(REGKEY, service);
 
-keys[4] = service_name;
+keys[4] = service;
 
 schSCManager = OpenSCManager(NULL, /* machine (NULL == local)*/
  NULL, /* database (NULL == 
default) */
  SC_MANAGER_ALL_ACCESS /* access required  
  */
 );
 
 if (!schSCManager)
 fprintf(stderr, "OpenSCManager failed\n");
 else {
-schService = OpenService(schSCManager, service_name, 
SERVICE_ALL_ACCESS);
+schService = OpenService(schSCManager, service, SERVICE_ALL_ACCESS);
 
 if (schService == NULL)
 fprintf(stderr, "OpenService failed\n");
 
 /* Could not open the service */
 else {
 /* try to stop the service */
 
 if (ControlService(schService, _WIN_SQUID_SERVICE_CONTROL_STOP,
&svcStatus)) {
 sleep(1);
 
 while (QueryServiceStatus(schService, &svcStatus)) {
 if (svcStatus.dwCurrentState == SERVICE_STOP_PENDING)
 sleep(1);
 else
 break;
 }
 }
 
 /* now remove the service */
 if (DeleteService(schService) == 0)
 fprintf(stderr, "DeleteService failed.\n");
 else
-printf("Service %s deleted successfully.\n", service_name);
+printf("Service " SQUIDSBUFPH " deleted successfully.\n", 
SQUIDSBUFPRINT(service_name));
 
 CloseServiceHandle(schService);
 }
 
 CloseServiceHandle(schSCManager);
 }
 }
 
 void
 WIN32_SetServiceCommandLine()
 {
-if (!service_name)
-service_name = xstrdup(APP_SHORTNAME);
+if (service_name.isEmpty())
+service_name = SBuf(APP_SHORTNAME);
 
-strcat(REGKEY, service_name);
+const char *service = servie_name.c_str();
+strcat(REGKEY, service);
 
-keys[4] = service_name;
+keys[4] = service;
 
 /* Now store the Service Command Line in the registry */
 WIN32_StoreKey(COMMANDLINE, REG_SZ, (unsigned char *) WIN32_Command_Line, 
strlen(WIN32_Command_Line) + 1);
 }
 
 void
 WIN32_InstallService()
 {
 SC_HANDLE schService;
 SC_HANDLE schSCManager;
 char ServicePath[512];
 char szPath[512];
 int lenpath;
 
-if (!service_name)
-service_name = xstrdup(APP_SHORTNAME);
+if (service_name.isEmpty())
+service_name = SBuf(APP_SHORTNAME);
 
-strcat(REGKEY, service_name);
+const char *service = service_name.c_str();
+strcat(REGKEY, service);
 
-keys[4] = service_name;
+keys[4] = service;
 
 if ((lenpath = GetModuleFileName(NULL, ServicePath, 512)) == 0) {

Re: [PATCH] validate -n parameter value

2014-06-16 Thread Kinkie 
LGTM, only:
Are the first two columns really necessary in the statement
::Parser::Tokenizer tok
?


On Sun, Jun 15, 2014 at 8:05 AM, Amos Jeffries  wrote:
> Update the service_name global to an SBuf and use Tokenizer to validate
> the -n argument is a pure alphanumeric.
>
> Amos



-- 
Francesco

Re: [PATCH] validate -n parameter value

2014-06-16 Thread Amos Jeffries 
On 16/06/2014 9:32 p.m., Kinkie wrote:
> LGTM, only:
> Are the first two columns really necessary in the statement
> ::Parser::Tokenizer tok
> ?
> 

Maybe not in this location. But its required in this "global" namespace
location other namespaces I've been using it there was clashes with
component-secific Parser classes. So I've been keeping consistency.

Amos

Re: [PATCH] validate -n parameter value

2014-06-16 Thread Alex Rousskov 
On 06/15/2014 12:05 AM, Amos Jeffries wrote:
> +if (optarg) {
> +SBuf t(optarg);
> +::Parser::Tokenizer tok(t);

Pleaser remove "t" as used-once and lacking a descriptive name. If you
insist on keeping it, please make it "const" and try to give it a more
descriptive name such as rawName or serviceName.


> +const CharacterSet chr = 
> CharacterSet::ALPHA+CharacterSet::DIGIT;

Is there a document stating that only those characters are valid? My
quick search resulted in [1] that seems to imply many other characters
are accepted. However, [2] lists more prohibited characters. Both seem
to allow "-" and "_" though. It would be good to provide a reference in
the code or commit message to explain why we are restricting its value.

Do you want to validate the total service name length? [1] says the
limit is 256 characters.

None of the above applies to -n used on Unix. Do we need to validate the
service name (whatever that is) there?


> +if (!tok.prefix(service_name, chr) || !tok.atEnd())

Please note that the above also rejects empty service names (-n "").
That may be good, but the error messages do not reflect that restriction.


> +fatal("Need to provide alphanumeric service name when 
> using -n option");
> +opt_signal_service = true;
> +} else {
> +fatal("Need to provide alphanumeric service name when using 
> -n option");
> +}

I recommend adjusting the fatal() messages to distinguish the two errors
and avoid a possible misunderstanding that the service name was provided
but was not alphanumeric in the second case:

  * Expected alphanumeric service name for the -n option but got: ...
  * A service name is required for the -n option.


I continue to dislike the undefined -n option meaning on non-Windows
boxes, but I have no objection to this patch and believe the above
changes can be done without a new review cycle.


Cheers,

Alex.
[1] http://msdn.microsoft.com/en-us/library/ms682450%28VS.85%29.aspx
[2] http://www.anzio.com/resources/cannot-install-or-start-windows-service

Re: [PATCH] validate -n parameter value

2014-06-22 Thread Amos Jeffries 
On 17/06/2014 4:06 a.m., Alex Rousskov wrote:
> On 06/15/2014 12:05 AM, Amos Jeffries wrote:
>> +if (optarg) {
>> +SBuf t(optarg);
>> +::Parser::Tokenizer tok(t);
> 
> Pleaser remove "t" as used-once and lacking a descriptive name. If you
> insist on keeping it, please make it "const" and try to give it a more
> descriptive name such as rawName or serviceName.


> 
>> +const CharacterSet chr = 
>> CharacterSet::ALPHA+CharacterSet::DIGIT;
> 
> Is there a document stating that only those characters are valid? My
> quick search resulted in [1] that seems to imply many other characters
> are accepted. However, [2] lists more prohibited characters. Both seem
> to allow "-" and "_" though. It would be good to provide a reference in
> the code or commit message to explain why we are restricting its value.
> 

Arbitrary design choice for guaranteed portability. Other characters can
be added if necessary, but most lack cross-platform usability for all
the situations the service name string is being used.


> Do you want to validate the total service name length? [1] says the
> limit is 256 characters.
> 
> None of the above applies to -n used on Unix. Do we need to validate the
> service name (whatever that is) there?

IMO portable consistency is better for this type of thing than being
pedantically accepting for the OS-specific character sets.

Yes, length needs to be checked. Thank you.

Does an arbitrary 32 bytes seem acceptible since this is a prefix on the
UDS sockets and paths which the final total still needs to fit within
the 256 or so for some OS?


> 
>> +if (!tok.prefix(service_name, chr) || !tok.atEnd())
> 
> Please note that the above also rejects empty service names (-n "").
> That may be good, but the error messages do not reflect that restriction.
> 
> 
>> +fatal("Need to provide alphanumeric service name when 
>> using -n option");
>> +opt_signal_service = true;
>> +} else {
>> +fatal("Need to provide alphanumeric service name when using 
>> -n option");
>> +}
> 
> I recommend adjusting the fatal() messages to distinguish the two errors
> and avoid a possible misunderstanding that the service name was provided
> but was not alphanumeric in the second case:
> 
>   * Expected alphanumeric service name for the -n option but got: ...
>   * A service name is required for the -n option.
> 

"" is not an alphanumeric string. But okay.

> 
> I continue to dislike the undefined -n option meaning on non-Windows
> boxes, but I have no objection to this patch and believe the above
> changes can be done without a new review cycle.
> 
> 
> Cheers,
> 
> Alex.
> [1] http://msdn.microsoft.com/en-us/library/ms682450%28VS.85%29.aspx
> [2] http://www.anzio.com/resources/cannot-install-or-start-windows-service
>

Re: [PATCH] validate -n parameter value

2014-07-14 Thread Amos Jeffries 
On 23/06/2014 12:35 a.m., Amos Jeffries wrote:
> On 17/06/2014 4:06 a.m., Alex Rousskov wrote:
>> On 06/15/2014 12:05 AM, Amos Jeffries wrote:
>>> +if (optarg) {
>>> +SBuf t(optarg);
>>> +::Parser::Tokenizer tok(t);
>>
>> Pleaser remove "t" as used-once and lacking a descriptive name. If you
>> insist on keeping it, please make it "const" and try to give it a more
>> descriptive name such as rawName or serviceName.
> 

No can do on removal. Without it we get:

error: request for member 'prefix' in 'tok', which is of non-class type
'Parser::Tokenizer(SBuf)'
 if (!tok.prefix(service_name, chr) || !tok.atEnd())
  ^
error: request for member 'atEnd' in 'tok', which is of non-class type
'Parser::Tokenizer(SBuf)'
 if (!tok.prefix(service_name, chr) || !tok.atEnd())
^

const seems to be fine though.

>>
>>> +const CharacterSet chr = 
>>> CharacterSet::ALPHA+CharacterSet::DIGIT;
>>
>> Is there a document stating that only those characters are valid? My
>> quick search resulted in [1] that seems to imply many other characters
>> are accepted. However, [2] lists more prohibited characters. Both seem
>> to allow "-" and "_" though. It would be good to provide a reference in
>> the code or commit message to explain why we are restricting its value.
>>
> 
> Arbitrary design choice for guaranteed portability. Other characters can
> be added if necessary, but most lack cross-platform usability for all
> the situations the service name string is being used.
> 
> 
>> Do you want to validate the total service name length? [1] says the
>> limit is 256 characters.
>>
>> None of the above applies to -n used on Unix. Do we need to validate the
>> service name (whatever that is) there?
> 
> IMO portable consistency is better for this type of thing than being
> pedantically accepting for the OS-specific character sets.
> 
> Yes, length needs to be checked. Thank you.
> 
> Does an arbitrary 32 bytes seem acceptible since this is a prefix on the
> UDS sockets and paths which the final total still needs to fit within
> the 256 or so for some OS?

Taking absence of a response as a yes. Fixed.


>>
>>> +if (!tok.prefix(service_name, chr) || !tok.atEnd())
>>
>> Please note that the above also rejects empty service names (-n "").
>> That may be good, but the error messages do not reflect that restriction.
>>

Moved this to an explicit check on *optarg content so it is obviously
intentional and gets the right "missing service name" error.

>>
>>> +fatal("Need to provide alphanumeric service name when 
>>> using -n option");
>>> +opt_signal_service = true;
>>> +} else {
>>> +fatal("Need to provide alphanumeric service name when 
>>> using -n option");
>>> +}
>>
>> I recommend adjusting the fatal() messages to distinguish the two errors
>> and avoid a possible misunderstanding that the service name was provided
>> but was not alphanumeric in the second case:
>>
>>   * Expected alphanumeric service name for the -n option but got: ...
>>   * A service name is required for the -n option.
>>
> 
> "" is not an alphanumeric string. But okay.
> 
>>
>> I continue to dislike the undefined -n option meaning on non-Windows
>> boxes, but I have no objection to this patch and believe the above
>> changes can be done without a new review cycle.

-n means the same on all boxes. Windows default is "squid" just like the
rest.

Updates made and applied to trunk.

Amos

Re: [PATCH] validate -n parameter value

2014-07-14 Thread Alex Rousskov 
On 07/14/2014 07:02 AM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>> I continue to dislike the undefined -n option meaning on non-Windows

> -n means the same on all boxes.


I am guessing that Windows have a standard concept of a "service" and
there is an interface for setting and getting the service name (and
possibly for other "service operations").

On Unix, a lot of things may be called services, and "service" term use
in interfaces such as ICAP and System V init scripts is standard, but I
cannot think of any formal Unix interface that matches what Squid is
doing with -n on Unix. We define -n as "service name to use for service
operations", but I do not know what those "service operations" are in a
Unix context. If most admins know what that means, there is no problem.


Hope this clarifies,

Alex.

Re: [PATCH] validate -n parameter value

2014-07-15 Thread Alex Rousskov 
On 07/14/2014 07:02 AM, Amos Jeffries wrote:
> On 23/06/2014 12:35 a.m., Amos Jeffries wrote:
>> On 17/06/2014 4:06 a.m., Alex Rousskov wrote:
>>> On 06/15/2014 12:05 AM, Amos Jeffries wrote:
 +if (optarg) {
 +SBuf t(optarg);
 +::Parser::Tokenizer tok(t);
>>>
>>> Pleaser remove "t" as used-once and lacking a descriptive name. If you
>>> insist on keeping it, please make it "const" and try to give it a more
>>> descriptive name such as rawName or serviceName.


> No can do on removal. Without it we get: 
> error: request for member 'prefix' in 'tok', which is of non-class type
> 'Parser::Tokenizer(SBuf)'


Yeah, we appear to hit some hairy C++ concept here that I cannot fully
reconstruct. I could not find an explanation by quick googling (all
results point to trivial cases of declaring a function instead of
calling the default constructor).

The following works for me:

  ::Parser::Tokenizer tok(SBuf(optarg, SBuf::npos));

However, at this point, it is not clear whether the extra "t" temporary
is actually worse than the extra SBuf argument! If you keep "t", a more
descriptive name for t would be nice but obviously not required.


 +if (!tok.prefix(service_name, chr) || !tok.atEnd())
>>>
>>> Please note that the above also rejects empty service names (-n "").
>>> That may be good, but the error messages do not reflect that restriction.
>>>
> 
> Moved this to an explicit check on *optarg content so it is obviously
> intentional and gets the right "missing service name" error.

The adjusted if condition in the committed code appears to dereference a
nil pointer when the service name is missing:

> if (optarg || *optarg == '\0') {

The code actually does not get that far because getopt() does not allow
a missing service name (at least in my tests), but the condition should
be fixed.


HTH,

Alex.

Re: [PATCH] validate -n parameter value

2014-07-18 Thread Amos Jeffries 
On 16/07/2014 10:53 a.m., Alex Rousskov wrote:
> On 07/14/2014 07:02 AM, Amos Jeffries wrote:
>> On 23/06/2014 12:35 a.m., Amos Jeffries wrote:
>>> On 17/06/2014 4:06 a.m., Alex Rousskov wrote:
 On 06/15/2014 12:05 AM, Amos Jeffries wrote:
> +if (optarg) {
> +SBuf t(optarg);
> +::Parser::Tokenizer tok(t);

 Pleaser remove "t" as used-once and lacking a descriptive name. If you
 insist on keeping it, please make it "const" and try to give it a more
 descriptive name such as rawName or serviceName.
> 
> 
>> No can do on removal. Without it we get: 
>> error: request for member 'prefix' in 'tok', which is of non-class type
>> 'Parser::Tokenizer(SBuf)'
> 
> 
> Yeah, we appear to hit some hairy C++ concept here that I cannot fully
> reconstruct. I could not find an explanation by quick googling (all
> results point to trivial cases of declaring a function instead of
> calling the default constructor).
> 
> The following works for me:
> 
>   ::Parser::Tokenizer tok(SBuf(optarg, SBuf::npos));
> 
> However, at this point, it is not clear whether the extra "t" temporary
> is actually worse than the extra SBuf argument! If you keep "t", a more
> descriptive name for t would be nice but obviously not required.
> 
> 
> +if (!tok.prefix(service_name, chr) || !tok.atEnd())

 Please note that the above also rejects empty service names (-n "").
 That may be good, but the error messages do not reflect that restriction.

>>
>> Moved this to an explicit check on *optarg content so it is obviously
>> intentional and gets the right "missing service name" error.
> 
> The adjusted if condition in the committed code appears to dereference a
> nil pointer when the service name is missing:
> 
>> if (optarg || *optarg == '\0') {
> 
> The code actually does not get that far because getopt() does not allow
> a missing service name (at least in my tests), but the condition should
> be fixed.

Ouch. Fixed now.


Amos