[dev.icinga.com #1047] check for empty command args in host|service|notification definitions

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/1047

Created by jw on 2010-12-08 16:48:42 +00:00

Assignee: mfriedrich
Status: Resolved (closed on 2011-01-19 09:50:25 +00:00)
Target Version: 1.3
Last Update: 2014-12-08 14:35:37 +00:00 (in Redmine)

Icinga Version: 1.10.0
OS Version: any

---

ido2db uses strtok to split the Command-String into Command and Arguments:

char *cmdptr = NULL;
char *argptr = NULL;

cmdptr = strtok(idi->buffered_input[IDO_DATA_HOSTCHECKCOMMAND], "!");
argptr = strtok(NULL, "\x0");

If there is no argument in the configuration, the second strtok returns NULL wich leads to segmentation faults(at least on solaris) if argptr got dereferenced later:

es[3] = ido2db_db_escape_string(idi, argptr);

The attached patch fixed that problem by setting the es[] strings to "\0" istead of escaping the NULL-ptr content if argptr is NULL

Attachments

• icinga_idoutils-nullptr.patch jw - 2010-12-08 16:48:42 +00:00 - patch for icinga-1.2.1.orig/module/idoutils/src/dbhandlers.c

Changesets

2011-01-05 16:20:51 +00:00 by mfriedrich 2fadc0c

idoutils: fix ido2db needs to check for empty command arguments on Solaris (Julian Wiesner) #1047

fixes #1047

2011-01-13 14:46:44 +00:00 by mfriedrich 22800c3

idoutils: fix ido2db needs to check for empty command arguments on Solaris (Julian Wiesner) #1047

fixes #1047

2011-01-19 09:18:07 +00:00 by mfriedrich e73f020

idoutils: fix check for empty command args in host|service|notification definitions #1047

this is clearly checking the values and putting an empty string instead of NULL
into asprintf, not violating the Solaris convention.

fixes #1047

2011-01-20 11:57:57 +00:00 by mfriedrich 6d5ba04

idoutils: fix check for empty command args in host|service|notification definitions #1047

this is clearly checking the values and putting an empty string instead of NULL
into asprintf, not violating the Solaris convention.

fixes #1047

[icinga-migration]

Updated by mfriedrich on 2010-12-17 16:19:01 +00:00

• Status changed from New to Assigned
• Assigned to set to mfriedrich
• Target Version changed from 1.2 (Stable) to 1.3

[icinga-migration]

Updated by mfriedrich on 2010-12-17 16:19:16 +00:00

thanks. i'll add that to my todo list.

[icinga-migration]

Updated by mfriedrich on 2011-01-05 16:21:22 +00:00

the main problem is that passing a NULL value to ido2db_db_escape_string will return a NULL value into es[] array member.

this will passed via adress pointer up into data[] which then will be processed within the ido2db_query_insert_or_update queries where the asprintf into a query buffer takes place. at least mysql and postgresql are using that (oracle uses binded params where this won't happen).

so the main problem is within there,

ido2db_query_insert_or_update_hostdefinition_definition_add
ido2db_query_insert_or_update_servicedefinition_definition_add
ido2db_query_insert_or_update_contactdefinition_notificationcommands_add
ido2db_query_insert_or_update_contactdefinition_notificationcommands_add

whereas those are defined in dbqueries.c. protecting asprintf won't make any sense as it will throw the segfault either way.

so i think your patch attempt is fine. it is applied in my mfriedrich/core branch and will run into testing, then merged into git master for 1.3 release.

thanks for the patch, more welcome :-)

[icinga-migration]

Updated by mfriedrich on 2011-01-05 18:09:50 +00:00

• Status changed from Assigned to Resolved
• Done % changed from 90 to 100

Applied in changeset commit:"2fadc0c66ea7a45f1f451409ab9180567bc673ce".

[icinga-migration]

Updated by mfriedrich on 2011-01-19 08:04:05 +00:00

• Status changed from Resolved to Assigned
• Done % changed from 100 to 50

i need to reopen this as it's not the correct solution.

passing \0 into a string causes this to be a string literal, on which a free() will segfault (at least on linux/unix based systems).

so the correct implementation would be to check if the value is NULL and if yes, add "" instead of NULL.

[icinga-migration]

Updated by mfriedrich on 2011-01-19 09:17:37 +00:00

• Subject changed from ido2db need to check for empty command arguments to check for empty command args in host|service|notification definitions

[icinga-migration]

Updated by mfriedrich on 2011-01-19 09:50:25 +00:00

• Status changed from Assigned to Resolved
• Done % changed from 50 to 100

Applied in changeset commit:"e73f0206e04f235fcabc9501f658c19b3158c27f".

[icinga-migration]

Updated by mfriedrich on 2014-12-08 14:35:37 +00:00

• Project changed from 18 to Core, Classic UI, IDOUtils
• Category set to IDOUtils
• Icinga Version set to 1
• OS Version set to any