[Cluster-devel] [PATCH 0/3] libcman: fix possible string nontermination

[Cluster-devel] [PATCH 0/3] libcman: fix possible string nontermination

[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(-)

-- 
1.7.11.4

[Cluster-devel] [PATCH 1/3] libcman: fix possible string nontermination: node name

[Jan Pokorný]

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? <jpokorny@redhat.com>
---
 cman/lib/libcman.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index 6ed8ecb..012047d 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -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)
-- 
1.7.11.4

[Cluster-devel] [PATCH 2/3] libcman: fix possible string nontermination: barrier name

[Jan Pokorný]

Similar to node name case (separate changeset).

Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
 cman/lib/libcman.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index 012047d..f27e726 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -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);
 }
-- 
1.7.11.4

[Cluster-devel] [PATCH 3/3] libcman: fix possible string nontermination: barrier name

[Jan Pokorný]

Similar to node name case (separate changeset).

Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
 cman/lib/libcman.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cman/lib/libcman.c b/cman/lib/libcman.c
index f27e726..d9d6c36 100644
--- a/cman/lib/libcman.c
+++ b/cman/lib/libcman.c
@@ -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);
 }
-- 
1.7.11.4

[Cluster-devel] [PATCH 0/3] libcman: fix possible string nontermination

[Christine Caulfield]

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.

On 11/10/12 15:21, Jan Pokorn? wrote:
> 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(-)
>