Call for inncheck testing

[Florian Schlichting]

Hi,

picking up from news.software.nntp, what do people here think about
uppercase vs lowercase letters in IPv6 addresses? I tend to think we
should be permissive, so 

--- a/scripts/inncheck.in
+++ b/scripts/inncheck.in
@@ -163,7 +163,7 @@ my $ip = '(?:25[0-5]|2[0-4][0-9]|[0-1]?[0-9]{1,2})';
 my $ipv4 = "$ip$dot$ip$dot$ip$dot$ip";
 my $ipv4_cidr = "$ip(?:$dot$ip){0,3}\\/[1-3]?\\d";
 my $ipv4_wildmat = '[\d\[\]\*]+(?:\.[\d\[\]\*]+){0,3}';
-my $ipv6 = '[\da-f:.]+'; # e.g. ::ffff:192.168.0.10
+my $ipv6 = '[\da-fA-F:.]+'; # e.g. ::ffff:192.168.0.10
 my $hostname = '[\w-]+|[\w.-]+\.[a-zA-Z]{2,}'; # hostname, FQDN
 my $hostnameRE = "(?:$hostname|$ipv4|$ipv6)";

for reference, what I wrote was:

> On 2011-07-07, D. Stussy wrote:
> > 1)  FAIL:  IPv6 address detection with PROPERLY capitalized hexidecimal.
> >   (I protested the RFC which recently changed this to "lowercase" digits
> > only.)
> 
> I don't really have an opinion on this one. Since uppercase letters work
> as well as lowercase ones, we could be permissive. Then again there is
> the RFC, and who are we to know better? inncheck is really just
> advisory, any admin could amend the regex or ignore that particular
> warning.
> 
> RFC 5952 section 4 / 4.3 says that all characters in an IPv6 address
> MUST be textually represented in lowercase and advises that humans
> should do so, too. At the same time, all implementations must be able to
> accept both lowercase and uppercase.


Then, I also wrote:

> > /var/lib/news/etc/readers.conf:10: not a valid option name: require_ssl
> > /var/lib/news/etc/readers.conf:10: option true must be immediately followed
> > by a colon
> > /var/lib/news/etc/readers.conf:10: not a valid option name: true
>
> BTW, do people think that the second and third line are ugly and
> misleading? I just thought of a small fix that would suppress further
> error messages until the parser is back in sync, at the danger of hiding
> two consecutive errors until the first one gets fixed...

I *do* find it ugly and misleading, and here's what I thought to do
about it:

--- a/scripts/inncheck.in
+++ b/scripts/inncheck.in
@@ -195,11 +195,13 @@ parse_config
     my @groups; # our stack of nested groups
     my %return; # flat hash of groups returned to caller for further examination
     my $group = { 'type' => '<global scope>', 'line' => 0, 'name' => '<global scope>' };
+    my $parse_error = 0; # stop printing errors when we've seen unknown text, until re-sync
 
     while (my $word = get_config_word()) {
         if (defined $options->{$word}) {
             # $word starts a new group definition: "peer news.example.com {"
             my ($name, $curly);
+            $parse_error = 0;
 
             eprint "$file:$line: cannot nest $word in $group->{'type'}!\n"
                 unless ($group->{'type'} eq 'group'
@@ -228,6 +230,7 @@ parse_config
         }
 
         if ($word eq '}') {
+            $parse_error = 0;
             if (scalar @groups == 0) {
                 eprint "$file:$line: extra closing brace";
             } else {
@@ -239,13 +242,14 @@ parse_config
 
         # include-file hack: ignore, user needs to check it separately
         if (defined $options->{'<include>'}->{$word}) {
+            $parse_error = 0;
             my $includefile = get_config_word();
             next;
         }
 
         # $word must be an option key by now
         eprint "$file:$line: option $word must be immediately followed by a colon\n"
-            unless $word =~ s/:$//;
+            unless $word =~ s/:$// or $parse_error;
         eprint "$file:$line: duplicate option $word in $group->{'type'} $group->{'name'}\n"
             if exists $group->{$word} and not defined $options->{'<multi>'}->{$word};
 
@@ -253,12 +257,15 @@ parse_config
                  : defined $options->{'<anywhere>'}->{$word}     ? $options->{'<anywhere>'}->{$word}
                  : undef;
         if ($type) {
+            $parse_error = 0;
             my $value = get_config_word();
             $group->{$word} = $value;
             eprint "$file:$line: not a valid $type: '$value'\n"
                 unless $value =~ /$type_regex{$type}/;
         } else {
-            eprint "$file:$line: not a valid option name: $word\n";
+            eprint "$file:$line: not a valid option name: $word\n"
+                unless $parse_error;
+            $parse_error = 1;
         }
     }
     while (scalar @groups > 0) {


and finally...

On Thu, Jul 07, 2011 at 11:14:52AM +0200, Julien ÉLIE wrote:
> >/etc/news/storage.conf:57: not a valid minsize[,maxsize] definition:
> >'16384,'
> >And the last line is probably due to a changed syntax requirement.
> 
> Or a parsing not as strict as it should be when INN reads the file?
> 
> storage/interface.c:
> 
>                   case SMsize:
>                     minsize = strtoul(p, NULL, 10);
>                     if ((p = strchr(p, ',')) != NULL) {
>                         p++;
>                         maxsize = strtoul(p, NULL, 10);
>                     }
>                     break;
> 
> so maxsize will be 0 and I read in the source code:
> 
>                   maxsize = 0; /* zero means no limit */
> 
> The behaviour is the expected one.

I'd think it's clearer to write

    size: 16384,

to indicate that it means "16384 and above", rather than without the
comma. And it's the same for mintime/maxtime, although that's hardly
ever used, no?


--- a/scripts/inncheck.in
+++ b/scripts/inncheck.in
@@ -176,8 +176,8 @@ my %type_regex = (
     'IPv6 address / "any / "none"'      => '^(?:' . $ipv6 . '|any|none)$',
     'list of hostnames'                 => '^'.$hostnameRE.'(?:\s*,\s*'.$hostnameRE.')*$',
     'list of hostnames or netblocks'    => '^'.$hostblockRE.'(?:\s*,\s*'.$hostblockRE.')*$',
-    'minsize[,maxsize] definition'      => '^\d+(?:,\d+)?$',
-    'mintime[,maxtime] definition'      => '^(?:\d+[Mdhms])+(?:,(?:\d+[Mdhms])+)?$',
+    'minsize[,maxsize] definition'      => '^\d+(?:,\d*)?$',
+    'mintime[,maxtime] definition'      => '^(?:\d+[Mdhms])+(?:,(?:\d+[Mdhms])*)?$',
     'number'                            => '^\d+$',
     'number / "unlimited" / "none"'     => '^(?:\d+|unlimited|none)$',
     'path'                              => '.*', # useful?


good night,
Florian