Re: [PATCH] validate -n parameter value

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
Received on Mon Jul 14 2014 - 13:03:13 MDT