this is quite serious; it'd be nice to fix before 3.3.0

Created attachment 4607 [details]
truncate without message fix

Hi,
this is a patch for the bug posted here. It also increases the buffer size to 150. So, the maximum character size for one line is now 148!
If configuration line exeeds the maximum then spamc quits with an error message.

Greeting NicoP.

Looks good to me (except for <CR>s in the patch).
I'd push the max line size to 255+2 though.

why not something much higher? 8192 for instance?

Please,

hold on. It's not that well done. I'll do another round.

Created attachment 4608 [details]
truncate without message fix V2

Ok. Here is the second version. Does apply to trunk (without my first patch). And again with the not wanted <CRs>. Sorry, don't know how to easily get rid of that.

First version had a problem that it exited to early (in case of -V).
Also, applied larger line maximum (257). But, that's your decision.

NicoP.

(In reply to comment #6)
> Created an attachment (id=4608) [details]
> truncate without message fix V2

+ if( (option[strlen(option)-1] != '\n') && (!feof(config)) ) {

Hmm, fgets could conceivably return an empty string (null at index 0),
so strlen(option)-1 could yield a -1. Better to be safe and test
the length first.

> Ok. Here is the second version. Does apply to trunk (without my first patch).
> And again with the not wanted <CRs>. Sorry, don't know how to easily get rid of
> that.

No problem, we just need to be careful when applying a patch.

> First version had a problem that it exited to early (in case of -V).

Thanks.

> Also, applied larger line maximum (257). But, that's your decision.

Good. I don't mind any reasonably large value. Justin's 8k is good too.

replace the corresponding while loop with the following:
while(!(feof(config)) && (fgets(option, CONFIG_MAX_LINE_SIZE, config) != NULL)) {

then I don't see any more case where fgets could return a null pointer.

Created attachment 4611 [details]
proposed patch

Please review and vote for the attached patch.
  (a slightly cleaned version of V2 (tabs, cr, option_l, 8kB)).

(In reply to comment #9)
> Created an attachment (id=4611) [details]
> proposed patch
> 
> Please review and vote for the attached patch.
>   (a slightly cleaned version of V2 (tabs, cr, option_l, 8kB)).

KAM +1

+1

Bug 6176: spamc truncates lines read from spamc.conf
Sending        spamc/spamc.c
Sending        spamc/spamc.h.in
Committed revision 891935.

Closing.

[warren@computer trunk]$ cat sample-nonspam.txt |spamc
[warren@computer trunk]$ echo $?
0

spamd -D sees no connection attempt from spamc.  Same result if spamd is running or not.

http://svn.apache.org/viewvc?view=revision&revision=891935
I backed out this patch and spamc works again.

NOTE: Output of nothing with zero return value means blackholing of mail.  Very dangerous.  This should be a very simple test-case possible with "make test".  A working spamd is not necessary to test this.

I backed this out just in case folks are dogfooding the trunk.

I am traveling most of Dec 21st.  I suppose rc1.proposed2 can be cut after you folks decide what to do about this bug.  I will be able to cut it on Tuesday, unless jm or somebody wants to do the cut on Monday.

Created attachment 4613 [details]
proposed patch, take 2

Apparently nobody is brave enough to touch the spamc.conf, so I'm
reluctantly doing it. Also fixed a buffer overflow when there
are more than 24 arguments present (command line + from a config file),
and fixed one spelling.

(In reply to comment #17)
> Created an attachment (id=4613) [details]
> proposed patch, take 2
> 
> Apparently nobody is brave enough to touch the spamc.conf, so I'm
> reluctantly doing it. Also fixed a buffer overflow when there
> are more than 24 arguments present (command line + from a config file),
> and fixed one spelling.

I hate to be annoying, but IMO it's critical that we come up with a .t script to exercise the failure case found in rc1.proposed1.

(In reply to comment #18)
> I hate to be annoying, but IMO it's critical that we come up with a .t script
> to exercise the failure case found in rc1.proposed1.

I've just done this:

: 106...; svn commit -m "bug 6176: add test script for error that broke rc1.proposed1"
Sending        MANIFEST
Adding         t/spamc_bug6176.t
Transmitting file data ..
Committed revision 893149.

(In reply to comment #17)
> Created an attachment (id=4613) [details]
> proposed patch, take 2
> 
> Apparently nobody is brave enough to touch the spamc.conf, so I'm
> reluctantly doing it. Also fixed a buffer overflow when there
> are more than 24 arguments present (command line + from a config file),
> and fixed one spelling.

+1

Tested in production and reviewed patch manually.

+1

I'm cutting rc1.proposed2 after you commit this, unless somebody suggests we delay for any particular other patch.

Bug 6176 spamc.c fix: spamc truncates lines read from spamc.conf (take two)
Sending        spamc/spamc.c
Sending        spamc/spamc.h.in
Transmitting file data ..
Committed revision 893270.

re-closing

Created attachment 4616 [details]
Enhanced error message

Attached is a little additional patch to spamc.c to distinguish a
line-too-long condition from a non-NL-terminated line, and issue an
appropriate diagnostic for each case, instead of always claiming
that line is too long.

Reopening for a small enhancement.

+1

(In reply to comment #26)
> +1

+1 KAM

Bug 6176: distinguish too long lines from non-NL-terminated (last) line
in the issued diagnostic
Sending spamc/spamc.c
Committed revision 897074.