[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [leafnode-list] Multi-level Local Newsgroups Name Bugs



"Bulgrien, Dennis" <Dennis.Bulgrien@xxxxxxxxxxxxxxxxxx> writes:

> Given Local Newsgroups:
>   x
>   x.y

I'd avoid that, although Leafnode should cope with it.

>   Clicking on x in Outlook Express says:

*grrr*

[...]
>     Jun 25 11:33:07 arendi leafnode[8251]: >240 Article posted, now be
> patient
>     Jun 25 11:33:07 arendi leafnode[8251]: >400 timeout - service
> discontinued
>     Jun 25 11:33:07 arendi leafnode[8252]: wrote
> /var/spool/news/x.y/.overview
>     Jun 25 11:33:10 arendi leafnode[7938]: <GROUP x
>     Jun 25 11:33:10 arendi leafnode[7938]: rereading
> /var/spool/news/leaf.node/groupinfo
>     Jun 25 11:33:10 arendi leafnode[7938]: >211 2 1 1 x group selected
>     Jun 25 11:33:10 arendi leafnode[7938]: <XOVER 1-1
>     Jun 25 11:33:10 arendi leafnode[7938]: >420 No articles in specified
> range.
>     Jun 25 11:37:40 arendi leafnode[8049]: >400 timeout - service
> discontinued
>
>   /var/spool/news/leaf.node/groupinfo:
>     x 0 0 993486787 ex
>     x.y 2 1 991834462 wy
          ^
Note the first article was stored as number 2. Article 1 has never
existed.

Computing the GROUP response, leafnode doesn't check if the group is
local and fakes a response suitable for a pseudo group.
When it comes to read the pseudo article, leafnode recognizes the
group as local.

My suggestion:

--- ../leafnode-2.0b8.orig/activutil.c  Wed Jan 10 10:16:06 2001
+++ activutil.c Mon Jun 25 21:55:00 2001
@@ -329,7 +329,7 @@
 	}
 	if ( g->first == 0 )
 	    g->first = 1;               /* pseudoarticle */
-	if ( g->last == 0 )
+	if ( g->last == 0 && !islocalgroup(g->name) )
 	    g->last = 1;
 	g->count = 0;
 	p = r;


IMHO empty groups should have the low water mark set to high+1:

--- ../leafnode-2.0b8.orig/localutil.c  Wed Jan 10 10:16:06 2001
+++ localutil.c Tue Jun 26 01:08:21 2001
@@ -63,6 +63,7 @@
 		syslog( LOG_ERR, "Not sufficient memory for newsgroup %s",
 				 name );
 		free( *a );
+		*a = NULL;
 		return;
 	    }
 	    return;
@@ -112,9 +113,9 @@
 	/* l points to group name, p to description */
 	if ( strlen(l) ) {
 	    if ( strlen(p) )
-		insertgroup( l, 0, 0, time(NULL), p );
+		insertgroup( l, 1, 0, time(NULL), p );
 	    else
-		insertgroup( l, 0, 0, time(NULL), "local group" );
+		insertgroup( l, 1, 0, time(NULL), "local group" );
 	    insertlocal( l );
 	}
     }

> Analysis I:
>   What leafnode did when the x.y message was posted is add both x and x.y to
> groupinfo and created their directories.  It should NOT have added x to
> groupinfo because x is empty and x in groupinfo causes the invalid XOVER.

Local groups must always be present in groupinfo. If you ever remove
an entry, new articles may get inserted with wrong article numbers.

The XOVER response was correct, but the GROUP response wasn't.

> Removing x from groupinfo stops the error (but every time a message is
> posted to a child group (ie. x.y), x is broken by it being readded to
> groupinfo):
>     Jun 25 11:50:49 arendi leafnode[8538]: rereading
> /var/spool/news/leaf.node/groupinfo
>     Jun 25 16:50:49 arendi leafnode[8538]: <MODE READER
>     Jun 25 11:50:49 arendi leafnode[8538]: >200 Leafnode 2.0b8, pleased to
> meet you!
>     Jun 25 11:50:49 arendi leafnode[8538]: <GROUP x
>     Jun 25 11:50:49 arendi leafnode[8538]: >211 2 0 0 x group selected

That output is correct. The "2" looks strange, but is no error.

[...]
>   Leafnode reports to newsreader the number of messages in group based on
> the number of files+directories in the local group dir (x/).  The newsreader
> subtracts from it the message range and reports the difference as new
> messages.  Leafnode should only count the number of files (not directories).

stat() over all directory entries would be far too expensive. And:

,----[ RFC 977 ]
| 3.2.1.  GROUP
|
|    GROUP ggg
[...]
|    The successful selection response will return the article numbers of
|    the first and last articles in the group, and an estimate of the
|    number of articles on file in the group.  It is not necessary that
|    the estimate be correct, although that is helpful; it must only be
|    equal to or larger than the actual number of articles on file.  (Some
|    implementations will actually count the number of articles on file.
|    Others will just subtract first article number from last to get an
|    estimate.)
`----

To improve the estimate, we could output no more than the difference
between last and first article number plus one:

--- ../leafnode-2.0b8.orig/nntpd.c      Wed Jan 10 10:16:06 2001
+++ nntpd.c     Tue Jun 26 02:27:07 2001
@@ -201,6 +201,7 @@
 	   syslog( LOG_DEBUG, "rereading %s", s );
 	readactive();
 	activetime = st.st_mtime;
+	readlocalgroups();
     }
 }
@@ -662,7 +663,7 @@
 		/* count articles in group */
 		i = 0;
 		if ( (d = opendir( "." )) != NULL ) {
-		    while ( ( de = readdir(d) ) != NULL ) {
+		    while ((g->first+i < g->last) && (de=readdir(d)) != NULL) {
 			if ( de->d_name[0] != '.' )
 			    i++;
 		    }
@@ -798,9 +799,11 @@
     }
 }

-void dolist( char *arg ) {
+void dolist( char *oarg ) {
     char * p = NULL;

+    char *arg = strdup(oarg);
+
     if ( !strcasecmp(arg, "extensions") ) {
 	nntpprintf("202 extensions supported follow");
 	printf(                      " OVER\r\n"
@@ -1961,8 +1964,8 @@

     pseudogroup = FALSE;

-    rereadactive();
     readlocalgroups();
+    rereadactive();

     signal( SIGCHLD, SIG_IGN );


(Beware of unsignedness issues...)

The first hunk tries to correct that local groups vanish from
active if there's no entry in groupinfo (yet). It causes syslog
messages, and therefor must be improved.

The bug "fixed" by the third hunk wasn't easy to find:
In dolist(), rereadactive() may be called, which implicitly calls
getaline(), which overwrites the buffer dolist()'s argument points to.
There are possibly similar bugs elsewhere.

The fourth hunk makes the change to activutil.c work.

It would also be legitimate to simply estimate article count doing:
        g->count = ((g->last > g->first) ? (g->last - g->first) : 0)
and forget about readdir().

There is still a small problem: leafnode will only update the
groupinfo file (and add new local groups) if an article is posted.

The above patches should eliminate the flaws you reported.

Stefan


-- 
leafnode-list@xxxxxxxxxxxxxxxxxxxxxxxxxxxx -- mailing list for leafnode
To unsubscribe, send mail with "unsubscribe" in the subject to the list