David Powell wrote:
> David Powell wrote:
>> Tony Nguyen wrote:
>>> Dave,
>>>
>>> See my responses inline and the updated webrev at:
>>>
>>> http://cr.opensolaris.org/~tonyn/firewall13Jan2009-inc/
>>> http://cr.opensolaris.org/~tonyn/firewall13Jan2009/
>>
>> I'm still working through ipf_include.sh.
>
> ... and servinfo.c. Here's the rest:
>
> servinfo.c:
>
> uaddr2port:
>
> If there's only one dot in the provided address, you'll scan past
> the beginning of your buffer (and could end up writing past the end
> of 'port_str'). You need to ensure 'p' doesn't advance past
> 'addr'.
>
Universal addresses should have the format
h1.h2.h3.h4.p1.p2 (IPv4)
x1:x2:x3:x4:x5:x6:x7:x8.p1.p2 (IPv6)
However, I agree that we should better handle bad input and will make
the suggested change.
> Since the string pointed to by 'p' is 'l' bytes long, after the
> strncpy 'port_str' won't neccesarily be NUL terminated, which could
> cause atol to fail.
I thought the prior bzero call would prevent that problem, especially if
we make sure 'p' doesn't advance past 'addr'. Or would you prefer should
be replacing it with "strlcpy(port_str, p, l + 1)" ?
> svc_getrpcinfo:
>
> I'm still wondering where the "12" in "buf[12]" comes from. It
> should either be documented, or a constant (one hopefully derived
> from whatever determines the length of the buffer).
Yes, a constant would be more clear but I'm not sure what the optimal
size should be. The buffer holds either port number (2^16 value) or a
netid (currently limited to tcp, udp, tcp6, udp6) and an extra space.
Does 12 seems reasonable?
> You now silently exclude protocols or ports that don't fit on the
> line. I suppose it is unlikely, but I'm wondering if that was a
> concious choice.
>
Yes, it is a conscious choice.
> printf("%s\n", str) is more simply written as puts(str);
>
will do
All suggested changes to ipf_include.sh will be followed.
Much appreciated,
tony
> ipf_include.sh:
>
> Don't use &&/|| with { }; use if/then instead.
>
> Instead of
>
> command && return 0 || return 1
>
> when command returns 0 or 1, do
>
> command
> return $?
>
> inst_ipf_file:
> inst_nat_file:
>
> Instead of echoing the result of fmri_to_file to concatenate a
> suffix, pass the suffix as an argument to fmri_to_file to append
> for you. Then just call fmri_to_file.
>
> (On further inspection, inst_nat_file doesn't appear to be used,
> and other uses of inst_ipf_file have been replaced per my previous
> comments. They should just be removed.)
>
> service_check_state:
>
> Instead of
>
> while true
> condition && break
> sleep
>
> do
>
> while !condition
> sleep
>
> file_get_ports:
> get_active_ports:
>
> '/to.*port =/ s/\(.*to.* port =\) \([a-z0-9]*\).*/\2/p'
>
> can be simplified to
>
> 's/.*to.* port = \([a-z0-9]*\).*/\1/p'
>
> since 'p' will only print if a substitution is made, and the
> pattern is more restrictive than the address.
>
> prepend_new_rules:
>
> I don't think it's necessary to escape '@'.
>
> tuple_get_port:
>
> There's no need to escape ':' in 's/.*\://''
>
> tuple_get_proto:
>
> There's no need to escape ':' in 's/\:.*//''
>
> Use if/then/else here; it will be clearer.
>
> ipf_remove_lock:
>
> Why do you remove the lock in two steps instead of just 'rm -r' as
> you do in ipf_get_lock?
>
> Instead of 'while [ true ]' use 'while :'.
>
> replace_file:
>
> Why not always move?
>
> process_server_svc:
>
> All callers test for IPF_FMRI before calling this function. You
> don't need to test it again.
>
> All callers test service_is_enabled before calling this function.
> You don't need to test it again.
>
> process_nonsvc_progs:
>
> Instead of using port_is_range, just 'set -- $port' and test $#.
>
> create_services_rules:
>
> Instead of scraping the output of svcs, it is probably more stable
> and more efficient to get the list of services using:
>
> svcprop -cf -p general/enabled -p general_ovr/enabled '*'
> 2>/dev/null |
> sed -n 's,^\(svc:.*\)/:properties/.* true$,\1,p' | sort -u
>
> Since your 'service_is_enabled' predicate determines if the service
> is enabled *or* temporarily enabled, with the above you can get rid
> of that test from your loop.
>
> service_update_rules:
>
> I missed that you don't install rules for services that are in
> maintenance. If this is the case, the decision to install rules
> for services that are temporarily disabled doesn't really make
> sense. (I'd still replace the use of svcs, though.)
>
> service_update:
> create_services_rules:
>
> Both test the result of ipf_get_lock, but afaikt it will always
> succeed.
