Completely stumped
Andreas Ericsson
ae at op5.se
Fri Jan 19 15:17:24 CET 2007
Tobias Klausmann wrote:
> Hi!
>
> On Thu, 18 Jan 2007, Andreas Ericsson wrote:
>>> The *only* thing I've left to try is removing the multiuser patch
>>> we talked about at the end of last year. If that does it, at
>>> least I have an idea *where* in the code my problem lies. I'll
>>> try that route tonight.
>>>
>> Which patch was this? I didn't find it in the december archives.
>
> It's the advanced permission patch as created by Altinity and
> Alex Burger. The thread has the subject "Advanced permissions".
> First mail is by me and has the message id
> 20061031095636.GA31688 at eric.schwarzvogel.de
>
> As far as I can tell, backdating from my own packages (2.6 with
> said patch) to dsitro packages (Gentoo, Nagios v2.5) fixed the
> problem. The new machine has run close to 12 hours without even
> remotely acting up. I've now updated to 2.6, still sans patch.
> I'll post in a few hours or sooner if I find out anything new.
>
> Also, I have a suspicion bout the reasons.
>
> Originally I didn't suspect the patch to be at fault: I only use
> it to regulate who can see what and who can use commands in the
> web interface. How that should affect the usual checks was beyond
> me. Added to that, we hadn't deplyed any production config with
> that feature in use - yet the production machine acted up.
>
> The other day I realized that the patch goes beyond what I was
> using: it also modifies notification behaviour. Looking back, I
> seem to recall that the degradation of check latency was coupled
> to the amount of notifications being sent. Unrelated to the
> Nagios troubles, we had some issues yesterday and Wednesday (with
> quite a few notifications being sent) and voila, the curves
> skyrocketed.
>
Was this by any chance coupled with a big fat spike of memory usage
on the Nagios server? I assume you do monitor memory usage, right?
> My C-fu is weak, so I ask those more versed in it to take a look
> at the patch. I'll also hand it to our local C guru, but he's
> quite swamped in work, so that may take some time.
>
Comments inline.
>
> diff -ur nagios-2.5.org/base/notifications.c nagios-2.5/base/notifications.c
> --- nagios-2.5.org/base/notifications.c 2006-04-07 18:24:13.000000000 -0400
> +++ nagios-2.5/base/notifications.c 2006-11-05 22:23:57.000000000 -0500
> @@ -832,7 +832,7 @@
> /* find all contacts for this service */
> for(temp_contact=contact_list;temp_contact!=NULL;temp_contact=temp_contact->next){
>
> - if(is_contact_for_service(svc,temp_contact)==TRUE)
> + if(is_contact_for_service_perm(svc,temp_contact,'n')==TRUE)
> add_notification(temp_contact);
> }
> }
> @@ -1572,7 +1572,7 @@
> /* get all contacts for this host */
> for(temp_contact=contact_list;temp_contact!=NULL;temp_contact=temp_contact->next){
>
> - if(is_contact_for_host(hst,temp_contact)==TRUE)
> + if(is_contact_for_host_perm(hst,temp_contact,'n')==TRUE)
> add_notification(temp_contact);
> }
> }
> diff -ur nagios-2.5.org/cgi/cgiauth.c nagios-2.5/cgi/cgiauth.c
> --- nagios-2.5.org/cgi/cgiauth.c 2006-10-08 19:35:18.000000000 -0400
> +++ nagios-2.5/cgi/cgiauth.c 2006-11-05 22:55:28.000000000 -0500
> @@ -218,7 +218,7 @@
> temp_contact=find_contact(authinfo->username);
>
> /* see if this user is a contact for the host */
> - if(is_contact_for_host(hst,temp_contact)==TRUE)
> + if(is_contact_for_host_perm(hst,temp_contact,'r')==TRUE)
> return TRUE;
>
> /* see if this user is an escalated contact for the host */
> @@ -295,14 +295,14 @@
> return FALSE;
>
> /* if this user is authorized for this host, they are for all services on it as well... */
> - if(is_authorized_for_host(temp_host,authinfo)==TRUE)
> - return TRUE;
> + /* if(is_authorized_for_host(temp_host,authinfo)==TRUE)
> + return TRUE;*/
>
> /* find the contact */
> temp_contact=find_contact(authinfo->username);
>
> /* see if this user is a contact for the service */
> - if(is_contact_for_service(svc,temp_contact)==TRUE)
> + if(is_contact_for_service_perm(svc,temp_contact,'r')==TRUE)
> return TRUE;
>
> /* see if this user is an escalated contact for the service */
> @@ -419,16 +419,16 @@
> if(temp_contact && temp_contact->can_submit_commands==FALSE)
> return FALSE;
>
> - /* see if this user is a contact for the host */
> - if(is_contact_for_host(temp_host,temp_contact)==TRUE)
> + /* see if this user is a contact for the host with permissions */
> + if(is_contact_for_host_perm(temp_host,temp_contact,'x')==TRUE)
> return TRUE;
>
> /* see if this user is an escalated contact for the host */
> if(is_escalated_contact_for_host(temp_host,temp_contact)==TRUE)
> return TRUE;
>
> - /* this user is a contact for the service, so they have permission... */
> - if(is_contact_for_service(svc,temp_contact)==TRUE)
> + /* see if this user is a contact for the service with permissions */
> + if(is_contact_for_service_perm(svc,temp_contact,'x')==TRUE)
> return TRUE;
>
> /* this user is an escalated contact for the service, so they have permission... */
> @@ -469,8 +469,8 @@
> if(temp_contact && temp_contact->can_submit_commands==FALSE)
> return FALSE;
>
> - /* this user is a contact for the host, so they have permission... */
> - if(is_contact_for_host(hst,temp_contact)==TRUE)
> + /* see if this user is a contact for the host with permissions */
> + if(is_contact_for_host_perm(hst,temp_contact,'x')==TRUE)
> return TRUE;
>
> /* this user is an escalated contact for the host, so they have permission... */
> diff -ur nagios-2.5.org/common/objects.c nagios-2.5/common/objects.c
> --- nagios-2.5.org/common/objects.c 2006-10-08 19:35:18.000000000 -0400
> +++ nagios-2.5/common/objects.c 2006-11-05 22:20:44.000000000 -0500
> @@ -4926,6 +4926,8 @@
> /* find a contact group from the list in memory */
> contactgroup * find_contactgroup(char *name){
> contactgroup *temp_contactgroup;
> + char *temp_contactgroup_name;
> + char *perms;
>
> #ifdef DEBUG0
> printf("find_contactgroup() start\n");
> @@ -4934,11 +4936,21 @@
> if(name==NULL || contactgroup_hashlist==NULL)
> return NULL;
>
> - for(temp_contactgroup=contactgroup_hashlist[hashfunc1(name,CONTACTGROUP_HASHSLOTS)];temp_contactgroup && compare_hashdata1(temp_contactgroup->group_name,name)<0;temp_contactgroup=temp_contactgroup->nexthash);
> + /* Ignore permissions */
> + temp_contactgroup_name = strdup(name);
> + perms = strchr(temp_contactgroup_name, ':');
> + if (perms)
> + *perms = '\0';
>
> - if(temp_contactgroup && (compare_hashdata1(temp_contactgroup->group_name,name)==0))
> + for(temp_contactgroup=contactgroup_hashlist[hashfunc1(temp_contactgroup_name,CONTACTGROUP_HASHSLOTS)];temp_contactgroup && compare_hashdata1(temp_contactgroup->group_name,temp_contactgroup_name)<0;temp_contactgroup=temp_contactgroup->nexthash);
> +
> + if(temp_contactgroup && (compare_hashdata1(temp_contactgroup->group_name,temp_contactgroup_name)==0))
> return temp_contactgroup;
>
This leaks sizeof(contactgroup) * persons_to_contact per notification. There needs
to be a
if (temp_contactgroup_name)
free(temp_contactgroup_name)
above that return (alhough the if()'s not strictly necessary as it's always true).
Alternatively, one could have
if (perm && !*perm)
*perm = ':';
and not bother with copying the string in the first place. It's not a const,
so modifying the struct directly is OK.
> + if(temp_contactgroup_name)
> + free(temp_contactgroup_name);
> +
Again, doesn't need if(). temp_contactgroup_name is set unconditionally.
> +
> #ifdef DEBUG0
> printf("find_contactgroup() end\n");
> #endif
> @@ -5427,7 +5439,9 @@
> int is_contact_for_host(host *hst, contact *cntct){
> contactgroupsmember *temp_contactgroupsmember;
> contactgroup *temp_contactgroup;
> -
> + char *temp_contactgroup_name;
> + char *perms;
> +
> if(hst==NULL || cntct==NULL){
> return FALSE;
> }
> @@ -5435,8 +5449,16 @@
> /* search all contact groups of this host */
> for(temp_contactgroupsmember=hst->contact_groups;temp_contactgroupsmember!=NULL;temp_contactgroupsmember=temp_contactgroupsmember->next){
>
> + /* Ignore permissions */
> + temp_contactgroup_name = strdup(temp_contactgroupsmember->group_name);
> + perms = strchr(temp_contactgroup_name, ':');
> + if (perms)
> + *perms = '\0';
> +
> /* find the contact group */
> - temp_contactgroup=find_contactgroup(temp_contactgroupsmember->group_name);
> + temp_contactgroup=find_contactgroup(temp_contactgroup_name);
> + if (temp_contactgroup_name)
> + free (temp_contactgroup_name);
> if(temp_contactgroup==NULL)
> continue;
Again, needs no if().
>
> @@ -5447,6 +5469,47 @@
> return FALSE;
> }
>
> +/* tests whether a contact is a contact for a particular host with write permissions */
> +int is_contact_for_host_perm(host *hst, contact *cntct, char perm){
> + contactgroupsmember *temp_contactgroupsmember;
> + contactgroup *temp_contactgroup;
> + char *temp_contactgroup_name;
> + char *perms;
> +
> + if(hst==NULL || cntct==NULL){
> + return FALSE;
> + }
> +
> + /* search all contact groups of this host */
> + for(temp_contactgroupsmember=hst->contact_groups;temp_contactgroupsmember!=NULL;temp_contactgroupsmember=temp_contactgroupsmember->next){
> +
> + /* Check for permissions */
> + temp_contactgroup_name = strdup(temp_contactgroupsmember->group_name);
This strdup() is unnecessary. A simple assignment would be enough, as the
variable is never modified.
> + perms = strchr(temp_contactgroup_name, ':');
> + if (perms) {
> + perms = strchr(perms, perm);
> + if (! (perms)) { /* permission not found so deny */
> + if (temp_contactgroup_name)
> + free(temp_contactgroup_name);
> + continue;
> + }
> + }
> +
> + /* No permissions set so defaulting to full access, or user has permission */
> +
> + /* find the contact group */
> + temp_contactgroup=find_contactgroup(temp_contactgroup_name);
This is buggy. temp_contactgroup will always be empty if perms is set. If
it's the other way around, it'll never match extended permissions anyway,
so it might as well return early.
> + if (temp_contactgroup_name)
> + free(temp_contactgroup_name);
> + if(temp_contactgroup==NULL)
> + continue;
> +
> + if(is_contact_member_of_contactgroup(temp_contactgroup,cntct)==TRUE)
> + return TRUE;
> + }
> +
> + return FALSE;
> + }
>
>
> /* tests whether or not a contact is an escalated contact for a particular host */
> @@ -5481,6 +5544,8 @@
> int is_contact_for_service(service *svc, contact *cntct){
> contactgroupsmember *temp_contactgroupsmember;
> contactgroup *temp_contactgroup;
> + char *temp_contactgroup_name;
> + char *perms;
>
> if(svc==NULL || cntct==NULL)
> return FALSE;
> @@ -5488,8 +5553,16 @@
> /* search all contact groups of this service */
> for(temp_contactgroupsmember=svc->contact_groups;temp_contactgroupsmember!=NULL;temp_contactgroupsmember=temp_contactgroupsmember->next){
>
> + /* Ignore permissions */
> + temp_contactgroup_name = strdup(temp_contactgroupsmember->group_name);
> + perms = strchr(temp_contactgroup_name, ':');
> + if (perms)
> + *perms = '\0';
> +
> /* find the contact group */
> - temp_contactgroup=find_contactgroup(temp_contactgroupsmember->group_name);
> + temp_contactgroup=find_contactgroup(temp_contactgroup_name);
> + if (temp_contactgroup_name)
> + free (temp_contactgroup_name);
> if(temp_contactgroup==NULL)
> continue;
>
> @@ -5500,6 +5573,47 @@
> return FALSE;
> }
>
> +/* tests whether a contact is a contact for a particular service */
> +int is_contact_for_service_perm(service *svc, contact *cntct, char perm){
> + contactgroupsmember *temp_contactgroupsmember;
> + contactgroup *temp_contactgroup;
> + char *temp_contactgroup_name;
> + char *perms;
> +
> + if(svc==NULL || cntct==NULL)
> + return FALSE;
> +
> + /* search all contact groups of this service */
> + for(temp_contactgroupsmember=svc->contact_groups;temp_contactgroupsmember!=NULL;temp_contactgroupsmember=temp_contactgroupsmember->next){
> +
> +
> + /* Check for permissions */
> + temp_contactgroup_name = strdup(temp_contactgroupsmember->group_name);
> + perms = strchr(temp_contactgroup_name, ':');
> + if (perms) {
> + perms = strchr(perms, perm);
> + if (! (perms)) { /* permission not found so deny */
> + if (temp_contactgroup_name)
> + free(temp_contactgroup_name);
> + continue;
> + }
> + }
> +
> + /* No permissions set so defaulting to full access, or user has permission */
> +
> + /* find the contact group */
> + temp_contactgroup=find_contactgroup(temp_contactgroup_name);
> + if (temp_contactgroup_name)
> + free (temp_contactgroup_name);
> + if(temp_contactgroup==NULL)
> + continue;
> +
> + if(is_contact_member_of_contactgroup(temp_contactgroup,cntct)==TRUE)
> + return TRUE;
> + }
> +
> + return FALSE;
> + }
>
>
> /* tests whether or not a contact is an escalated contact for a particular service */
>
>
All in all, I'd advice against using this patch, or at least try without it
first thing you do in case you run into problems.
--
Andreas Ericsson andreas.ericsson at op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Nagios-users mailing list
Nagios-users at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nagios-users
::: Please include Nagios version, plugin version (-v) and OS when reporting any issue.
::: Messages without supporting info will risk being sent to /dev/null
More information about the Users
mailing list