Stricter validation of article numbers

[ewxrjk]

This brings the implementation largely in line with RFC3977, and fixes anomalous behavior when very large article numbers are passed in NNTP commands, e.g. OVER reduces the article number mod 2^32 and then returns an unexpected range of articles.

Adds tests for IsValidArticleNumber and IsValidRange.

[ewxrjk]

PS. The OVER issue was discovered using https://github.com/ewxrjk/inntest. https://gist.github.com/ewxrjk/2b5d401958ddc853c36ac6d35731221b shows an example of the current behaviour.

[Julien-Elie]

Thanks Richard for having caught that bug. Note that the build with "make warnings" triggers an error. Also, I think support/mkmanifest should mention tests/lib/artnumber.t ("make check-manifest" should not return any differences, otherwise snapshot generation will fail).

[Julien-Elie]

It is true that lib/artnumber.c would have been a better name than lib/numbers.c; I was not very inspired when adding these checks long time ago. I think I'll rename it after your bug fix (which incidentally deserves a note in doc/pod/news.pod as it is a real fix for OVER and maybe other commands).

[ewxrjk]

Thanks for the feedback! I believe all issues are addressed by the latest push.

[Julien-Elie]

Unfortunately, a gcc warning also appears when running the test suite. Thanks for your last push anyway.

[ewxrjk]

Ah, sorry, I'd not spotted that the CI rules weren't quite the same as the default local build rules. I might start a discussion about that on inn-workers. Anyway, that should be fixed now.

[Julien-Elie]

Thanks for the new patch, I'll integrate it soon. For the build rules, we enforce in CI the use of "make warnings".

[Julien-Elie]

Incidentally, as for your previous PR "innd: more conservative signal mask handling", when using the Merge function from GitHub, it generated 2 commits (yours, and one of merge) in the main branch. I cherry-picked the commit for the 2.6 branch and it made 1 commit. Is the merge commit necessary in the main branch? What is its use? Seems everything would work fine without it but I do not see an option not to do it from the GitHub web interface.

[ewxrjk]

I don't see any benefit to the merge commit. I'm not sure if it can be suppressed via the GitHub UI.

One option is to do the merge manually, i.e. using the git command line. Normally git merge will do a fast-forward (i.e. no merge commit) if possible.

[Julien-Elie]

OK thanks, I'll give a try with direct git commands with this PR. Besides, I can do more check consistencies and update possible other files (like NEWS) on the fly just after the integration of your commit. (I am still more accustomed to doing that after more than a decade of using SVN for INN.)

[Julien-Elie]

Committed to main (f2adebc9392b9266fedefd19f819e5d620dfa8df).

[Julien-Elie]

For the record, the previous behaviour:

[C] OVER 2147483649-2147483673
[S] 423 No articles in 2147483649-2147483673
[C] OVER 4294967297-4294967321
[S] 224 Overview information for 4294967297-4294967321 follows
[S] 1 ...
[S] 2 ...

Now with this patch, the responses are correct:

[S] 501 Syntax error in range