[prev in list] [next in list] [prev in thread] [next in thread] 

List:       squid-dev
Subject:    Re: [PATCH] validate -n parameter value
From:       Amos Jeffries <squid3 () treenet ! co ! nz>
Date:       2014-07-18 11:52:24
Message-ID: 53C90A78.1070908 () treenet ! co ! nz
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]