Re: [PATCH] validate -n parameter value

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
>
Received on Sun Jun 22 2014 - 12:36:00 MDT