headers
Matija Nalis | 1 Sep 2011 16:08
Picon

Re: cant store article: bogus Xref: header in INN 2.5 ?

On Wed, Aug 31, 2011 at 09:47:48PM +0200, Julien ?LIE wrote:
> Hi Matija,
> > Aug 30 05:52:54 news2 innd: SERVER bad_format newsfeed.CARNet.hr
> > Aug 30 05:52:54 news2 innd: SERVER cant store article: bogus Xref: header
> > 
> > Aug 31 03:14:37.637 -
newsfeed.carnet.hr<fe278512-d9d1-4b46-976a-1cf252417112 <at> g30g2000vbu.googlegroups.com>  403
cant store article
> > 
> > sm `grephistory '<fe278512-d9d1-4b46-976a-1cf252417112 <at> g30g2000vbu.googlegroups.com>'` |
grep Xref
> > Xref:  newsfeed.CARNet.hr sci.med.cardiology:211617 alt.support.diabetes:452777
misc.health.alternative:332298 alt.christnet.prayer:66586
> > 
> > Note the two spaces after "Xref:".
> 
> It could explain the problem.  I see in storage/tradspool/tradspool.c:
> 
> Yet, it does not explain why the error did not happen with an INN 2.4.6 xrefslave
> as the same code is present in the 2.4.6 version.

Don't know. I just doublechecked on third server (also xrefslave as
the upgraded one, but still running 2.4.6-snapshot-20090119) and it
shows no such errors in the logs (and it gets articles from the same 
"newsfeed" server as the upgraded one).

> > As some other random OK messages I
> > looked at give just one space after "Xref:", so that might be the cause.
> > Why it happens so sporadically I don't know...
>
(Continue reading)

Permalink | Reply | 0 comments |
headers
Julien ÉLIE | 1 Sep 2011 20:23
Favicon

Re: cant store article: bogus Xref: header in INN 2.5 ?

Hi Russ,

>> It would be better to find out why two spaces are inserted, instead of
>> modifying CrackXref to handle that weirdness.
> 
> It would be nice to fix both, but in general we shouldn't be this picky
> about whitespace.  It's pretty easy to skip over leading whitespace in
> that header, and in general RFC 5322 headers are not supposed to care
> about whitespace.  The RFC 5536 grammar also explicitly allows leading
> whitespace:
> 
>     The Xref header field indicates where an article was filed by the last
>     news server to process it.  User agents often use the information in
>     the Xref header field to avoid multiple processing of crossposted
>     articles.
> 
>     xref            =  "Xref:" SP *WSP server-name
>                        1*( FWS location ) *WSP CRLF

Yes, you're totally right.  We should fix how the Xref: header is parsed
by tradspool.
A slave INN server could be fed by another news server so we cannot assume any
restrictive syntax for the Xref: header field.
Besides, even when the upstream server is INN, there is an issue with the Xref:
parsing because of folding whitespace that is not taken into account (innd
properly folds the Xref: header it generates in ARTassignnumbers).

Suggested fix for INN 2.5.3:

--- storage/tradspool/tradspool.c	(r?vision 9364)
(Continue reading)

Permalink | Reply | 7 comments |
headers
Matija Nalis | 1 Sep 2011 23:57
Picon

Re: cant store article: bogus Xref: header in INN 2.5 ?


On Thu, Sep 01, 2011 at 08:15:34PM +0200, Julien ÉLIE wrote:
> So, hmm...  Maybe you could upgrade your newsfeed server to 2.5.2-2~squeeze1;
> I bet it will directly solve the issue for your slave server :-)

I do understand that would be easier way out -- if your bet is correct, but as
I only have redundancy for xrefslave servers and not for the feeder, I'd very
much first like to test and then let users hammer those for at least a month
or two before trying to upgrade newsfeed (so I can fallback to 2.4.6 server if 
anything go wrong -- as I'm doing just now).

I guess I'm just not much of a betting man :-)

> Otherwise, you could patch your slave server with a pending patch I will post
> in my next message.

I'll try that, thanks. I hope your patch against 2.5.3 will apply against
Debians 2.5.2 source (we'll see soon, hopefully tommorow).

With the "bogus Xref: header" problem found, what still worries me is why
the server sometimes (but not always) decided to throttle with the
"Interrupted system call writing SMstore file -- throttling" in the same
second as "bogus Xref: header" hit?

More precisely, would fixing the "bogus Xref: header" also fix all cases
leading to server auto-throttling, or may there be there some other possibile
vectors for server to auto-throttle when receiving just one invalid article?

--

-- 
Opinions above are GNU-copylefted.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Matija Nalis | 2 Sep 2011 17:13
Picon

Re: cant store article: bogus Xref: header in INN 2.5 ?

On Thu, Sep 01, 2011 at 08:23:43PM +0200, Julien ??LIE wrote:
> Yes, you're totally right.  We should fix how the Xref: header is parsed
> by tradspool.
> 
> Suggested fix for INN 2.5.3:
> 
> --- storage/tradspool/tradspool.c	(r?vision 9364)
> +++ storage/tradspool/tradspool.c	(copie de travail)

I've replaced CrackXref() for Debian Squeeeze INN 2.5.2-2~squeeze1,
but it broke horribly. 

I do not know if it is related to this patch or general
miscompilation (although it looked good, I didn't try to recompile
pure Debian INN2 first -- will do that soon).

The sympthoms is that my xrefslave server started to create bunch of
junk-named (either binary junk, or sometimes directories looking like
part of the headers or body, like "Path", "wrote", etc) directories
in /var/spool/news/articles, and in them (sometimes directly,
sometimes under more junk-named subdirs) files called "0",
which contains the article which look normal, for example with Xref:
newsfeed.CARNet.hr rec.music.beatles:441370

also, innd started throwing lots of errors like:
Sep  2 16:42:41 rana innd: tradspool: could not symlink
/var/spool/news/articles/soc/history/medieval/279460 to /var/spool/news/articles/wrote/0:
File exists

I backed up to Debian version until I can investigate more next week.
(Continue reading)

Permalink | Reply | 6 comments |
headers
Julien ÉLIE | 2 Sep 2011 19:27
Favicon

Re: cant store article: bogus Xref: header in INN 2.5 ?

Hi Matija,

> I've replaced CrackXref() for Debian Squeeeze INN 2.5.2-2~squeeze1,
> but it broke horribly.

I am terribly sorry.  It is also what I have discovered in my news 
server this evening, coming back home.

> I do not know if it is related to this patch

Yes it is.

The problem is that I thought CrackXref() was given a pointer to a 
string (the Xref: header field value).  It appears that it is a pointer 
inside the article, without boundaries after the end of the Xref: header 
field.  With skip_fws(), we reach the start of the body (as Xref: is 
usually the last header).

> I backed up to Debian version until I can investigate more next week.

I have a new patch.
I will post it tomorrow, after having checked that everything is all 
right.  Sorry for having suggested you a broken patch yesterday.

I see that we have other parsing issues in the same file.
For instance again the Xref: header field, where tabs are not checked. 
Neither are folding whitespace.

             if (innconf->storeonxref) {
                 /* skip path element */
(Continue reading)

Permalink | Reply | 5 comments |
headers
Julien ÉLIE | 2 Sep 2011 21:28
Favicon

Re: cant store article: bogus Xref: header in INN 2.5 ?

> 235 Article transferred OK
> head <ITv0a$3test <at> news.trigofacile.com4>
> 221 0 <ITv0a$3test <at> news.trigofacile.com4>  head
> Path: news.trigofacile.com2!pouet!test
> From: =?ISO-8859-15?Q?Julien_=C9LIE?= <iulius <at> nom-de-mon-site.com.invalid>
> Newsgroups: trigofacile.test
> Subject: test
> Date: Wed, 30 Aug 2011 16:19:01 +0200 (CEST)
> Message-ID: <ITv0a$3test <at> news.trigofacile.com10>
> Xref:      plop.plop.test    trigofacile.test:510
> .

The message-ID was of course ending with "4>" in the HEAD response.
(I changed the numbers in my mail to homogeneize the different results.)

--

-- 
Julien ÉLIE

« Encore un peu, et cette histoire finira en queue de poisson ! »
   (Astérix)
Permalink | Reply | 0 comments |
headers
Julien ÉLIE | 4 Sep 2011 11:10
Favicon

Re: procbatch

Hi Florian,

> tonight I got around to finishing the procbatch manual page, which
> you'll find attached

Many thanks for this new documentation.  It is very useful to have.
There was a lack in INN.

> I just wondered if we should perhaps check if -s / -t are being used 
> without -c /-m, and report an error (warn? die?) in that case?

Yes, patch applied.

> =head1 SYNOPSIS
> 
> B<procbatch> [B<-e> I<peer>] [B<-d> I<outdir>] [B<-c>] [B<-s>
> I<spooldir>] [B<-m>] [B<-t> I<backlogdir>] [B<-u>] F<batchfile>

I also added B<-h>, B<-q> and B<-v> to this synopsis line.

> If there are non-zero length files, innfeed has dropped some
> articles, and those dropped article files have to be manually
> processed or those articles will never be sent to peers. To do that,
> first make sure that the file doesn't correspond to a currently
> running innfeed, for example by calling C<ctlinnd flush innfeed!>

That has changed since INN 2.5.0; news.daily now takes care of these
dropped files.

Reworded to:
(Continue reading)

Permalink | Reply | 0 comments |
headers
Julien ÉLIE | 5 Sep 2011 00:56
Favicon

Re: cant store article: bogus Xref: header in INN 2.5 ?

Hi all,

> I have a new patch.  I will post it tomorrow, after having checked
> that everything is all right.

I believe we need a complete fix for all the uses of the Xref: header 
field.  I came up with a partial patch, only fixing the issue you 
mentioned.  However, other places should be fixed in the code.

It means that it will probably delay the release of INN 2.5.3.  Though I 
am not very enthusiastic about modifying how the Xref: header is parsed 
in critical programs just before the release of a new version, I think 
it should be done because bad things can occur with the current state of 
the parsing.
As Matija discovered, a 2.5 slave fed by a 2.4 upstream leads to issues. 
  We have to change the Xref: parsing in both tradspool storage and 
overview data.

Other affected programs parsing Xref: header fields, are:
archive, expire, makehistory, overchan, and also nnrpd (because of 
virtual hosting and NEWNEWS checks).

They use overview data, and for most of them also rely on the fact that 
no leading whitespaces exist in overview fields.
Slaves are broken.  (Not an upstream INN 2.5 because the behaviour is 
consistent:  no leading whitespaces are present in generated Xref: 
header fields.  And previously existing articles were integrated without 
any leading whitespaces in overview.)

Another point:
(Continue reading)

Permalink | Reply | 4 comments |
headers
Russ Allbery | 5 Sep 2011 02:29
Picon
Favicon
Gravatar

Re: cant store article: bogus Xref: header in INN 2.5 ?

Julien ÉLIE <julien <at> trigofacile.com> writes:

> Another point:

>    xref            =  "Xref:" SP *WSP server-name
>                       1*( FWS location ) *WSP CRLF

> I plan on allowing FWS instead of *WSP before server-name.  As it happens
> when parsing an already injected/accepted article, I believe it does no
> harm (contrary to nnrpd, acting as an injecting agent; and anyway, it will
> not accept articles already containing an Xref: header field).

That's certainly fine for ease of maintenance.  The only reason why the
grammar specifies *WSP there instead of [FWS] was to follow the general
dictate of section 2.2:

   o  Every line of a header field body (including the first and any
      that are subsequently folded) MUST contain at least one non-
      whitespace character.

         NOTE: This means that no header field body defined by or
         referenced by this document can be empty.  As a result, rather
         than using the <unstructured> syntax from Section 3.2.5 of
         [RFC5322], this document uses a stricter definition:

   unstructured    =  *WSP VCHAR *( [FWS] VCHAR ) *WSP

         NOTE: The [RFC5322] specification sometimes uses [FWS] at the
         beginning or end of ABNF describing header field content.  This
         specification uses *WSP in such cases, also in cases where this
(Continue reading)

Permalink | Reply | 0 comments |
headers
Matija Nalis | 7 Sep 2011 15:48
Picon

Re: cant store article: bogus Xref: header in INN 2.5 ?

On Mon, Sep 05, 2011 at 12:56:42AM +0200, Julien ?LIE wrote:
> Hi all,
>
>> I have a new patch.  I will post it tomorrow, after having checked
>> that everything is all right.
>
> I believe we need a complete fix for all the uses of the Xref: header  
> field.  I came up with a partial patch, only fixing the issue you  
> mentioned.  However, other places should be fixed in the code.

Would it work to fix this one reported issue and not lead to new
problems? If so, can you send me that partial patch so I can see how
it fares?

I'd rather run with debian version + small patch, than to have to
repackage whole latest version...

> It means that it will probably delay the release of INN 2.5.3.  Though I  
> am not very enthusiastic about modifying how the Xref: header is parsed  
> in critical programs just before the release of a new version, I think  

Well, you can always release 2.5.3 now without this fix, and then add
full fix in next version when it can be better tested.

> it should be done because bad things can occur with the current state of  
> the parsing.
> As Matija discovered, a 2.5 slave fed by a 2.4 upstream leads to issues.  
>  We have to change the Xref: parsing in both tradspool storage and  
> overview data.
>
(Continue reading)

Permalink | Reply | 2 comments |

Gmane