Skip to contentSkip to conversation

libcman: fix possible string nontermination

PATCH New Jan Pokorný 2 participants 5 messages Oct 11, 2012

lore
Share & tools
lore

Jan Pokorný

author
#0 lore raw

cover libcman: fix possible string nontermination

Hello once more,

I discovered cases potentially leading to string overruns later in the processing.

Jan Pokorn? (3):
libcman: fix possible string nontermination: node name
libcman: fix possible string nontermination: barrier name
libcman: fix possible string nontermination: barrier name

 cman/lib/libcman.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Signature · 1 line
1.7.11.4
author
#1 lore raw

PATCH 1/3 libcman: fix possible string nontermination: node name

Haven't tested it, but it seems that if node.cn_name has 254 non-null bytes (should be otherwise perfectly valid, actual characters number may vary due to utf-8), it will pester later in the processing due to not being null-terminated (depends whether 255th byte being accidentally zero), strcmp in find_node_by_name being the first troublesome place in row.

After this change and taking preceding condition into account, the situation should be safe.

Signed-off-by Jan Pokorn? <[email protected]>
---
 cman/lib/libcman.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
cman/lib/libcman.c +1 -1
index 6ed8ecb..012047d 100644
@@ -685,7 +685,7 @@ int cman_get_node(cman_handle_t handle, int nodeid, cman_node_t *node)
 	}
 
 	cman_node.node_id = nodeid;
-	strncpy(cman_node.name, node->cn_name, sizeof(cman_node.name) - 1);
+	strncpy(cman_node.name, node->cn_name, sizeof(cman_node.name));
 	status = info_call(h, CMAN_CMD_GETNODE, &cman_node, sizeof(struct cl_cluster_node),
 			   &cman_node, sizeof(struct cl_cluster_node));
 	if (status < 0)
Signature · 1 line
1.7.11.4
author
#2 lore raw

PATCH 2/3 libcman: fix possible string nontermination: barrier name

In reply to Jan Pokorný

Similar to node name case (separate changeset).

Signed-off-by Jan Pokorn? <[email protected]>
---
 cman/lib/libcman.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
cman/lib/libcman.c +4 -4
index 012047d..f27e726 100644
@@ -912,7 +912,7 @@ int cman_barrier_register(cman_handle_t handle, const char *name, int flags, int
 	}
 
 	binfo.cmd = BARRIER_CMD_REGISTER;
-	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
+	strncpy(binfo.name, name, sizeof(binfo.name));
 	binfo.arg = nodes;
 	binfo.flags = flags;
 

@@ -933,7 +933,7 @@ int cman_barrier_change(cman_handle_t handle, const char *name, int flags, int a
 	}
 
 	binfo.cmd = BARRIER_CMD_CHANGE;
-	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
+	strncpy(binfo.name, name, sizeof(binfo.name));
 	binfo.arg = arg;
 	binfo.flags = flags;
 

@@ -954,7 +954,7 @@ int cman_barrier_wait(cman_handle_t handle, const char *name)
 	}
 
 	binfo.cmd = BARRIER_CMD_WAIT;
-	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
+	strncpy(binfo.name, name, sizeof(binfo.name));
 
 	return info_call(h, CMAN_CMD_BARRIER, &binfo, sizeof(binfo), NULL, 0);
 }

@@ -972,7 +972,7 @@ int cman_barrier_delete(cman_handle_t handle, const char *name)
 	}
 
 	binfo.cmd = BARRIER_CMD_DELETE;
-	strncpy(binfo.name, name, sizeof(binfo.name) - 1);
+	strncpy(binfo.name, name, sizeof(binfo.name));
 
 	return info_call(h, CMAN_CMD_BARRIER, &binfo, sizeof(binfo), NULL, 0);
 }
Signature · 1 line
1.7.11.4
author
#3 lore raw

PATCH 3/3 libcman: fix possible string nontermination: barrier name

In reply to Jan Pokorný

Similar to node name case (separate changeset).

Signed-off-by Jan Pokorn? <[email protected]>
---
 cman/lib/libcman.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
cman/lib/libcman.c +1 -1
index f27e726..d9d6c36 100644
@@ -1131,6 +1131,6 @@ int cman_node_fenced(cman_handle_t handle, int nodeid, uint64_t time, char *agen
 
 	f.nodeid = nodeid;
 	f.fence_time = time;
-	strncpy(f.fence_agent, agent, sizeof(f.fence_agent) - 1);
+	strncpy(f.fence_agent, agent, sizeof(f.fence_agent));
 	return info_call(h, CMAN_CMD_UPDATE_FENCE_INFO, &f, sizeof(f), NULL, 0);
 }
Signature · 1 line
1.7.11.4
NAK
#4 lore raw

cover libcman: fix possible string nontermination

In reply to Jan Pokorný

in RHEL6 those strings are copied using strcpy - I think it was changed in the STABLE branch to strncpy after a coverity scan

TBH the original strcpy is quite safe as the strings come from internal sources and are pre-validated.

Anyway, the arrays are allocated as size+1 so the strings will never overflow.

NACK.

Quoted 13 lines · Jan Pokorn?

Hello once more,

I discovered cases potentially leading to string overruns later in the processing.

Jan Pokorn? (3):
libcman: fix possible string nontermination: node name
libcman: fix possible string nontermination: barrier name
libcman: fix possible string nontermination: barrier name

  cman/lib/libcman.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)