Merge pull request #1052 from kgaillot/fixes

PE memory management fixes

configure.ac

@@ -1965,6 +1965,7 @@ lrmd/Makefile                                                   \
         lrmd/pacemaker_remote.service				\
         lrmd/pacemaker_remote					\
 extra/Makefile							\
+	extra/alerts/Makefile					\
 	extra/resources/Makefile				\
 	extra/logrotate/Makefile				\
 	extra/logrotate/pacemaker				\

crmd/lrm.c

@@ -499,7 +499,7 @@ get_rsc_metadata(const char *type, const char *rclass, const char *provider, boo
         return NULL;
     }
 
-    snprintf(key, len, "%s::%s:%s", type, rclass, provider);
+    snprintf(key, len, "%s::%s:%s", rclass, provider, type);
     if(force == FALSE) {
         metadata = g_hash_table_lookup(metadata_hash, key);
         if (metadata) {
@@ -509,21 +509,18 @@ get_rsc_metadata(const char *type, const char *rclass, const char *provider, boo
 
     if(metadata == NULL) {
         rc = lrm_state_get_metadata(lrm_state, rclass, provider, type, &metadata, 0);
-        crm_trace("Retrieved live metadata for %s: %s (%d)", key, pcmk_strerror(rc), rc);
         if(rc == pcmk_ok) {
+            crm_trace("Retrieved live metadata for %s", key);
             CRM_LOG_ASSERT(metadata != NULL);
             g_hash_table_insert(metadata_hash, key, metadata);
             key = NULL;
         } else {
-            CRM_LOG_ASSERT(metadata == NULL);
-            metadata = NULL;
+            crm_trace("No metadata found for %s: %s" CRM_XS " rc=%d",
+                     key, pcmk_strerror(rc), rc);
+            CRM_CHECK(metadata == NULL, metadata = NULL);
         }
     }
 
-    if (metadata == NULL) {
-        crm_warn("No metadata found for %s: %s (%d)", key, pcmk_strerror(rc), rc);
-    }
-
     free(key);
     return metadata;
 }
@@ -751,17 +748,17 @@ build_operation_update(xmlNode * parent, lrmd_rsc_info_t * rsc, lrmd_event_data_
 
     m_string = get_rsc_metadata(rsc->type, rsc->class, rsc->provider, safe_str_eq(op->op_type, RSC_START));
     if(m_string == NULL) {
-        crm_err("No metadata for %s::%s:%s", rsc->provider, rsc->class, rsc->type);
+        crm_err("No metadata for %s::%s:%s", rsc->class, rsc->provider, rsc->type);
         return TRUE;
     }
 
     metadata = string2xml(m_string);
     if(metadata == NULL) {
-        crm_err("Metadata for %s::%s:%s is not valid XML", rsc->provider, rsc->class, rsc->type);
+        crm_err("Metadata for %s::%s:%s is not valid XML", rsc->class, rsc->provider, rsc->type);
         return TRUE;
     }
 
-    crm_trace("Including additional digests for %s::%s:%s", rsc->provider, rsc->class, rsc->type);
+    crm_trace("Including additional digests for %s::%s:%s", rsc->class, rsc->provider, rsc->type);
     append_restart_list(op, metadata, xml_op, caller_version);
     append_secure_list(op, metadata, xml_op, caller_version);
 

crmd/notify.c

@@ -756,6 +756,7 @@ crmd_notify_fencing_op(stonith_event_t * e)
     set_alert_key_int(CRM_notify_rc, e->result);
 
     send_notifications("fencing");
+    free(desc);
 }
 
 void

cts/CIB.py

@@ -414,8 +414,7 @@ def add_resources(self):
 [Service]
 Type=notify
 ExecStart=/usr/bin/python -c 'import time, systemd.daemon; time.sleep(10); systemd.daemon.notify("READY=1"); time.sleep(86400)'
-ExecStop=/bin/sleep 10
-ExecStop=/bin/kill -s KILL \$MAINPID
+ExecStop=/bin/sh -c 'sleep 10; [ -n "\$MAINPID" ] && kill -s KILL \$MAINPID'
 """
 
             os.system("cat <<-END >/tmp/DummySD.service\n%s\nEND" % (dummy_service_file))

cts/HBDummy.in

@@ -48,19 +48,6 @@ failure()
 	echo -ne "[FAILED]\r"
 }
 
-# rpm based distros
-if [ -d @sysconfdir@/sysconfig ]; then
-	[ -f @INITDIR@/functions ] && . @INITDIR@/functions
-	[ -f @sysconfdir@/sysconfig/pacemaker ] && . @sysconfdir@/sysconfig/pacemaker
-	[ -z "$LOCK_FILE" ] && LOCK_FILE="@localstatedir@/lock/subsys/pacemaker"
-fi
-
-# deb based distros
-if [ -d @sysconfdir@/default ]; then
-	[ -f @sysconfdir@/default/pacemaker ] && . @sysconfdir@/default/pacemaker
-	[ -z "$LOCK_FILE" ] && LOCK_FILE="@localstatedir@/lock/pacemaker"
-fi
-
 dummy_usage() {
 	cat <<END
 usage: $0 dummyarg1 dummyarg2 {start|stop|status}

cts/LSBDummy.in

@@ -45,19 +45,6 @@ failure()
 	printf "[FAILED]\r"
 }
 
-# rpm based distros
-if [ -d @sysconfdir@/sysconfig ]; then
-	[ -f @INITDIR@/functions ] && . @INITDIR@/functions
-	[ -f @sysconfdir@/sysconfig/pacemaker ] && . @sysconfdir@/sysconfig/pacemaker
-	[ -z "$LOCK_FILE" ] && LOCK_FILE="@localstatedir@/lock/subsys/pacemaker"
-fi
-
-# deb based distros
-if [ -d @sysconfdir@/default ]; then
-	[ -f @sysconfdir@/default/pacemaker ] && . @sysconfdir@/default/pacemaker
-	[ -z "$LOCK_FILE" ] && LOCK_FILE="@localstatedir@/lock/pacemaker"
-fi
-
 dummy_usage() {
 	cat <<END
 usage: $0 {start|stop|status}

doc/Pacemaker_Explained/en-US/Ch-Alerts.txt

@@ -162,14 +162,11 @@ instance attributes are completely up to the particular agent.
 
 == Using the Sample Alert Agents ==
 
-Several sample alert agents are provided in the
-https://github.com/ClusterLabs/pacemaker/tree/master/extra/alerts[+extra/alerts+]
-directory of the pacemaker source tree. If you installed Pacemaker via a
-package, these might be available somewhere on your system, such as
-+/usr/share/pacemaker+.
-
-While these sample scripts may be used directly as alert agents,
-they are provided mainly as templates to be edited to suit your purposes.
+Pacemaker provides several sample alert agents, installed in
++/usr/share/pacemaker/alerts+ by default.
+
+While these sample scripts may be copied and used as-is, they are provided
+mainly as templates to be edited to suit your purposes.
 See their source code for the full set of instance attributes they support.
 
 .Sending cluster events as SNMP traps
@@ -178,8 +175,8 @@ See their source code for the full set of instance attributes they support.
 -----
 <configuration>
     <alerts>
-        <alert id="snmp_alert" path="/path/to/pcmk_snmp_helper.sh">
-            <instance_attributes id="config_for_snmp_helper">
+        <alert id="snmp_alert" path="/path/to/alert_snmp.sh">
+            <instance_attributes id="config_for_alert_snmp">
                 <nvpair id="trap_node_states" name="trap_node_states" value="all"/>
             </instance_attributes>
             <meta_attributes id="config_for_timestamp">
@@ -199,8 +196,8 @@ See their source code for the full set of instance attributes they support.
 -----
     <configuration>
         <alerts>
-            <alert id="smtp_alert" path="/path/to/pcmk_smtp_helper.sh">
-              <instance_attributes id="config_for_pcmk_smtp_helper">
+            <alert id="smtp_alert" path="/path/to/alert_smtp.sh">
+              <instance_attributes id="config_for_alert_smtp">
                   <nvpair id="email_sender" name="email_sender"
                       value="donotreply@example.com"/>
               </instance_attributes>
@@ -307,6 +304,14 @@ Special concerns when writing alert agents:
   recommended to configure +sudo+ to allow the agent to run the necessary
   commands as another user with the appropriate privileges.
 
+* As always, take care to validate and sanitize user-configured parameters,
+  such as CRM_alert_timestamp (whose content is specified by the
+  user-configured timestamp-format), CRM_alert_recipient, and all instance
+  attributes. Mostly this is needed simply to protect against configuration
+  errors, but if some user can modify the CIB without having hacluster-level
+  access to the cluster nodes, it is a potential security concern as well, to
+  avoid the possibility of code injection.
+
 [NOTE]
 =====
 The alerts interface is designed to be backward compatible with the external

doc/Pacemaker_Remote/en-US/Ch-Options.txt

@@ -92,8 +92,10 @@ remote node must share the same private key. By default, this
 key is placed at +/etc/pacemaker/authkey+ on each node.
 
 You can change the default port and/or key location for Pacemaker and
-pacemaker_remote via environment variables. These environment variables can be
-enabled by placing them in the +/etc/sysconfig/pacemaker+ file.
+pacemaker_remote via environment variables. How these variables are set varies
+by OS, but usually they are set in the +/etc/sysconfig/pacemaker+ or
++/etc/default/pacemaker+ file.
+
 ----
 #==#==# Pacemaker Remote
 # Use a custom directory for finding the authkey.

extra/Makefile.am

@@ -18,7 +18,7 @@
 
 MAINTAINERCLEANFILES    = Makefile.in
 
-SUBDIRS                 =  resources logrotate
+SUBDIRS                 =  alerts resources logrotate
 
 mibdir = $(datadir)/snmp/mibs
 mib_DATA = PCMK-MIB.txt

extra/alerts/Makefile.am

@@ -0,0 +1,22 @@
+#
+# Copyright (C) 2016 Ken Gaillot <kgaillot@redhat.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+#
+
+MAINTAINERCLEANFILES    = Makefile.in
+
+samplesdir = $(datadir)/$(PACKAGE)/alerts/
+samples_DATA = alert_file.sh.sample alert_smtp.sh.sample alert_snmp.sh.sample

extra/alerts/pcmk_alert_sample.sh → extra/alerts/alert_file.sh.sample

@@ -17,19 +17,12 @@
 # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 #
 ##############################################################################
-# This sample script assumes that only users who already have root access can
-# edit the CIB. Otherwise, a malicious user can create damage anywhere in the
-# filesystem  where user hacluster has access - as well as writing to special
-# files.
-# If that is not the case in your environment, you should edit this script to
-# validate the log-destination.
-#
 # Sample configuration (cib fragment in xml notation)
 # ================================
 # <configuration>
 #   <alerts>
-#     <alert id="alert_sample" path="/path/to/pcmk_alert_sample.sh">
-#       <instance_attributes id="config_for_pcmk_alert_sample">
+#     <alert id="alert_sample" path="/path/to/alert_file.sh">
+#       <instance_attributes id="config_for_alert_file">
 #         <nvpair id="debug_option_1" name="debug_exec_order" value="false"/>
 #       </instance_attributes>
 #       <meta_attributes id="config_for_timestamp">
@@ -41,12 +34,15 @@
 # </configuration>
 
 if [ -z $CRM_alert_version ]; then
-    echo "Pacemaker version 1.1.15 is required" >> ${CRM_alert_recipient}
+    echo "Pacemaker version 1.1.15 or later is required" >> "${CRM_alert_recipient}"
     exit 0
 fi
 
 debug_exec_order_default="false"
 
+# Pacemaker passes instance attributes to alert agents as environment variables.
+# It is completely up to the agent what instance attributes to support.
+# Here, we define an instance attribute "debug_exec_order".
 : ${debug_exec_order=${debug_exec_order_default}}
 
 if [ "${debug_exec_order}" = "true" ]
@@ -62,7 +58,7 @@ fi
 
 case $CRM_alert_kind in
     node)
-        echo "${tstamp}Node '${CRM_alert_node}' is now '${CRM_alert_desc}'" >> ${CRM_alert_recipient}
+        echo "${tstamp}Node '${CRM_alert_node}' is now '${CRM_alert_desc}'" >> "${CRM_alert_recipient}"
         ;;
     fencing)
         # Other keys:
@@ -71,7 +67,7 @@ case $CRM_alert_kind in
         # CRM_alert_task
         # CRM_alert_rc
         #
-        echo "${tstamp}Fencing ${CRM_alert_desc}" >> ${CRM_alert_recipient}
+        echo "${tstamp}Fencing ${CRM_alert_desc}" >> "${CRM_alert_recipient}"
         ;;
     resource)
         # Other keys:
@@ -95,12 +91,12 @@ case $CRM_alert_kind in
         case ${CRM_alert_desc} in
             Cancelled) ;;
             *)
-                echo "${tstamp}Resource operation '${CRM_alert_task}${CRM_alert_interval}' for '${CRM_alert_rsc}' on '${CRM_alert_node}': ${CRM_alert_desc}${CRM_alert_target_rc}" >> ${CRM_alert_recipient}
+                echo "${tstamp}Resource operation '${CRM_alert_task}${CRM_alert_interval}' for '${CRM_alert_rsc}' on '${CRM_alert_node}': ${CRM_alert_desc}${CRM_alert_target_rc}" >> "${CRM_alert_recipient}"
                 ;;
         esac
         ;;
     *)
-        echo "${tstamp}Unhandled $CRM_alert_kind alert" >> ${CRM_alert_recipient}
-        env | grep CRM_alert >> ${CRM_alert_recipient}
+        echo "${tstamp}Unhandled $CRM_alert_kind alert" >> "${CRM_alert_recipient}"
+        env | grep CRM_alert >> "${CRM_alert_recipient}"
         ;;
 esac

extra/alerts/pcmk_smtp_helper.sh → extra/alerts/alert_smtp.sh.sample

@@ -22,13 +22,13 @@
 # ================================
 # <configuration>
 #   <alerts>
-#     <alert id="smtp_alert" path="/path/to/pcmk_smtp_helper.sh">
-#       <instance_attributes id="config_for_pcmk_smtp_helper">
+#     <alert id="smtp_alert" path="/path/to/alert_smtp">
+#       <instance_attributes id="config_for_alert_smtp">
 #         <nvpair id="cluster_name" name="cluster_name" value=""/>
 #         <nvpair id="email_client" name="email_client" value=""/>
 #         <nvpair id="email_sender" name="email_sender" value=""/>
 #       </instance_attributes>
-#       <recipient id="smtp_destination" value="admin@where.admin.lives"/>
+#       <recipient id="smtp_destination" value="admin@example.com"/>
 #     </alert>
 #   </alerts>
 # </configuration>
@@ -56,10 +56,8 @@ if [ ! -z "${email_recipient##*@*}" ]; then
 fi
 
 if [ -z ${CRM_alert_version} ]; then
-    email_subject="Pacemaker version 1.1.15 is required for smtp-helper"
-
+    email_subject="Pacemaker version 1.1.15 or later is required for alerts"
 else
-
     case ${CRM_alert_kind} in
         node)
             email_subject="${CRM_alert_timestamp} ${cluster_name}: Node '${CRM_alert_node}' is now '${CRM_alert_desc}'"
@@ -92,21 +90,24 @@ else
             ;;
 
     esac
+fi
 
-    if [ ! -z "${email_subject}" ]; then
-        case $email_client in
-            sendmail)
-                sendmail -t -r ${email_sender} <<__EOF__
+if [ ! -z "${email_subject}" ]; then
+    case $email_client in
+        # This sample script supports only sendmail for sending the email.
+	# Support for additional senders can easily be added by adding
+	# new cases here.
+        sendmail)
+            sendmail -t -r "${email_sender}" <<__EOF__
 From: ${email_sender}
 To: ${email_recipient}
 Return-Path: ${email_sender}
 Subject: ${email_subject}
 
 ${email_body}
 __EOF__
-                ;;
-            *)
-                ;;
-        esac
-    fi
+            ;;
+        *)
+            ;;
+    esac
 fi