-------- Original-Nachricht -------- > Datum: Mon, 04 Feb 2008 22:38:35 -0800 > Von: Jason Axley <[EMAIL PROTECTED]> > An: Steve <[EMAIL PROTECTED]> > CC: [EMAIL PROTECTED], [email protected] > Betreff: Re: [dspam-dev] Re: [dspam-users] CVS updates
> Steve wrote: > > -------- Original-Nachricht -------- > > > >> Datum: Sun, 03 Feb 2008 23:21:29 -0800 > >> Von: Jason Axley <[EMAIL PROTECTED]> > >> An: Mick Johnson <[EMAIL PROTECTED]> > >> CC: [email protected], > [EMAIL PROTECTED], Julien Valroff <[EMAIL PROTECTED]> > >> Betreff: Re: [dspam-users] CVS updates > >> > > > > > >> Hey Mick, > >> > >> > > Hallo Jason > > Hallo Jason > > > > > >> I have the attached patch to the mysql driver that should go in. It's > >> part of the 3.8.1cvs build on debian/ubuntu and fixes some problematic > >> bugs that break global groups that would be good to include. I don't > >> believe it has been included in the kirya.net build yet, but I've been > >> running it on my busy server and it is like a dream... > >> > >> > > I don't see the benefit of this patch. The only benefit I see is that > the LOGDEBUG message will print the proper name in the error message. That's > all. > > > That's a bit harsh Steve. > Sorry. I did not wanted to sound harsh. > It does a bit more than that. > Yes. It does more than that. I see it. But what problem does it try to solve? > And > admittedly, it may not fix the root cause of the problem but it fixes > the problematic result that I was seeing because of the broken logic: > the functions in the driver were returning EINVAL when problems looking > up the group with getpwnam. The driver treats that as a fatal error > instead of continuing on and processing > Yes. You are right. The question I have: Situation: Error's in looking up groups or group membership 1) Possible result: DSPAM is failing. 2) Possible result: DSPAM continues. The patch you sent does the 2nd. It just ignores group membership and continues without printing out any errors. It reads up tokens for just the user. So the merged group functionality is off. I personally would like DSPAM to not break (aka solution 2) but I want it to act always the same (aka solution 1). > - if (CTX->flags & DSF_MERGED) { > + if (CTX->group != NULL && CTX->flags & DSF_MERGED) { > > Note the null check that I inserted so it will skip the body of the > branch. > Yes. I noticed that. My question to this: In what case is a user member of a merged group but CTX->group is NULL? If you have that case, then some thing is ultra fishy with your DSPAM. I can understand that capturing the case that getpwnam returns nothing is desirable but attribute group being empty while the user is member of a merged group is very strange. Have you seen such a case in production? How could that happen? > I changed all occurrences of the code that were _not_ checking for a > null group (even though the code below does this) to avoid returning > EINVAL if the group can't be looked up. Why the group ID is sometimes > NULL is something that still deserves looking into. > CTX->group is not the group ID. The group ID gets queried by reading pw_uid of the passwd struct: p = _mysql_drv_getpwnam (CTX, CTX->group); gid = p->pw_uid; > But with this > patch, it seems to work like a charm with my config. > I can imagine that. It just does not use merged groups and falls back to normal lookups if there is a problem with getpwnam and/or the group. > This bug caused dspam to deliver scads of spam without tagging it or > doing anything, which was flooding people's inboxes with spam that > couldn't be processed. At least now it can use the user's data to tag > the messages... > Okay. I understand. > > For example (original code): > > > >> int uid = -1, gid = -1; > >> > > [..] > > > >> if (!CTX->group || CTX->flags & DSF_MERGED) > >> p = _mysql_drv_getpwnam (CTX, CTX->username); > >> else > >> p = _mysql_drv_getpwnam (CTX, CTX->group); > >> > >> > > Here you would add a variable name having either the username or the > group name. > > > > > >> if (p == NULL) > >> { > >> LOGDEBUG ("_mysql_drv_get_spamtotals: unable to > _mysql_drv_getpwnam(%s)", > >> CTX->username); > >> > >> > > Here you would print out the CORRECT name used. The original code would > always print out the user name but NEVER the group name. Your patch fixes > that. > > > > I was getting _mysql_drv_get_spamtotals: unable to > _mysql_drv_getpwnam(NULL) messages because of that which was not helping > debugging. Fixing this error helps with debugging, as many of the other > debug statements I added do as well. > > > > > >> if (!(CTX->flags & DSF_MERGED)) > >> return EINVAL; > >> } else { > >> > >> uid = p->pw_uid; > >> } > >> > >> if (CTX->flags & DSF_MERGED) { > >> p = _mysql_drv_getpwnam (CTX, CTX->group); > >> if (p == NULL) > >> { > >> LOGDEBUG ("_mysql_drv_getspamtotals: unable to > _mysql_drv_getpwnam(%s)", > >> CTX->group); > >> return EINVAL; > >> } > >> > >> } > >> > >> gid = p->pw_uid; > >> > >> > > Your patch would not jump into the above code block if group is NULL and > you run merged groups. It would as well NOT get the gid. In your case gid > would be -1 if some one is not running merged group. > > > Right, not desirable but at least the error conditions are handled as > transient and not fatal so the system doesn't cease to function just > because of this. There seem to still be problems with merged groups > somewhere. > I don't think that merged groups are the problem. It is the whole getpwnam and the passwd struct (and the caching of the passwd struct in the CVS edition). > > > > > >> snprintf (query, sizeof (query), > >> "select uid, spam_learned, innocent_learned, " > >> "spam_misclassified, innocent_misclassified, " > >> "spam_corpusfed, innocent_corpusfed, " > >> "spam_classified, innocent_classified " > >> " from dspam_stats where (uid = %d or uid = %d)", > >> uid, gid); > >> > >> > > Even if group would be empty (having -1 as value) the above SQL query > would only return user data since negative gid's are not possible/normal. > > > > > > Could you explain me what the benefits are of the patch set? What case > does the patch handle which is not handled in the original code (beside the > proper LOGDEBUG message)? > > > > > > > See above ;-) > :) > And, it adds additional debug messages to the get_pwnam function to aid > in determining where the calls were returning NULL. Turns out they were > returning null...because NULL was being passed in sometimes for the > group name... I didn't have time to find out what code was goofing that > up, and this fixed the logic in the driver so that the bleeding stopped > enough to avoid me ripping dspam out entirely and replacing it with > something else... > I understand. From that perspective the patch set is doing his job. > >> -Jason. > >> Steve > >> > > Steve > > > > > > > >> Mick Johnson wrote: > >> > >>> All > >>> > >>> A few pending updates have been pushed up to CVS : > >>> > >>> * Allow users to select multiple rows by clicking > >>> on the initial row, holding shift, and clicking on the final row. > >>> * Adds a "select 200" button to the quarantine page. > >>> * Removed some junk from a previous merge. > >>> > >>> The Feature Request page has also been updated - the donations button > >>> > >> and > >> > >>> dollar value components have been removed as we're no longer accepting > >>> donations for this project. > >>> > >>> A few of the patches came in via this interface, this actually makes > it > >>> harder to patch as a) I don't know who submitted them and can't bring > up > >>> > >> any > >> > >>> suggestions or corrections directly, and b) they have less visibility > on > >>> this list. In the future, if you wish to submit patches (and I'm > always > >>> happy when people do) please do so here to ensure the community can > >>> > >> review. > >> > >>> The latest CVS version seems to have been running stably for some time > >>> > >> now > >> > >>> and I'm looking to push this out as a stable 3.8.1 this month unless I > >>> > >> hear > >> > >>> otherwise. > >>> > >>> Finally, looking forward to a great 2008! > >>> > >>> Cheers > >>> Mick Johnson > >>> Sensory Networks > >>> > >>> > >>> > >>> > >>> > >>> > > > > > -- Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen! Ideal für Modem und ISDN: http://www.gmx.net/de/go/smartsurfer
