Ocfs2 devel - Jul 2010 - [patch] ocfs2: tighten up strlen() checking

This function is only called from one place and it's like this:
	dlm_register_domain(conn->cc_name, dlm_key, &fs_version);

The "conn->cc_name" is 64 characters long.  If
strlen(conn->cc_name)
were equal to O2NM_MAX_NAME_LEN (64) that would be a bug because
strlen() doesn't count the NULL character.

In fact, if you look how O2NM_MAX_NAME_LEN is used, it mostly describes
64 character buffers.  The only exception is nd_name from struct
o2nm_node.

Anyway I looked into it and in this case the domain string comes from
osb->uuid_str in ocfs2_setup_osb_uuid().  That's 32 characters and NULL
which easily fits into O2NM_MAX_NAME_LEN.  This patch doesn't change how
the code works, but I think it makes the code a little cleaner.

Signed-off-by: Dan Carpenter <error27 at gmail.com>
---
Or we could get rid of check entirely.

diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
index 6b5a492..084b051 100644
--- a/fs/ocfs2/dlm/dlmdomain.c
+++ b/fs/ocfs2/dlm/dlmdomain.c
@@ -1671,7 +1671,7 @@ struct dlm_ctxt * dlm_register_domain(const char *domain,
 	struct dlm_ctxt *dlm = NULL;
 	struct dlm_ctxt *new_ctxt = NULL;
 
-	if (strlen(domain) > O2NM_MAX_NAME_LEN) {
+	if (strlen(domain) >= O2NM_MAX_NAME_LEN) {
 		ret = -ENAMETOOLONG;
 		mlog(ML_ERROR, "domain name length too long\n");
 		goto leave;

Hi Dan,

I think O2NM_MAX_NAME_LEN is the max valid length of the domain name.
Regarding your patch, it changed to be that a domain name with length
O2NM_MAX_NAME_LEN (NULL character not included) is not permitted.

Though that check seems useless for current calls, we'd better keep it.

Checking the structure,

 99 struct ocfs2_cluster_connection {
100         char cc_name[GROUP_NAME_MAX];
101         int cc_namelen;

cc_name is not a NULL tailed string. the cc_namelen specifies the length of it.
There does is misuse of cc_name, such as


   7    832  fs/ocfs2/stack_user.c <<user_cluster_connect>>
             rc = dlm_new_lockspace(conn->cc_name, strlen(conn->cc_name),

   5    308  fs/ocfs2/stack_o2cb.c <<o2cb_cluster_connect>>
             dlm = dlm_register_domain(conn->cc_name, dlm_key,
&fs_version);

Also, the uuid shouldn't be treated as NULL tailed string.
142 struct ocfs2_control_message_down {
143         char    tag[OCFS2_CONTROL_MESSAGE_OP_LEN];
144         char    space1;
145         char    uuid[OCFS2_TEXT_UUID_LEN];
146         char    space2;

thus,
the calling of 
fs/ocfs2/stack_user.c:474:      ocfs2_control_send_down(msg->uuid, nodenum);
   ->ocfs2_connection_find(uuid)
	->size_t len = strlen(name);

is suspectable.

Could you please make patch for that instead?

regards,
wengang.

On 10-07-10 16:33, Dan Carpenter wrote:
> This function is only called from one place and it's like this:
> 	dlm_register_domain(conn->cc_name, dlm_key, &fs_version);
> 
> The "conn->cc_name" is 64 characters long.  If
strlen(conn->cc_name)
> were equal to O2NM_MAX_NAME_LEN (64) that would be a bug because
> strlen() doesn't count the NULL character.
> 
> In fact, if you look how O2NM_MAX_NAME_LEN is used, it mostly describes
> 64 character buffers.  The only exception is nd_name from struct
> o2nm_node.
> 
> Anyway I looked into it and in this case the domain string comes from
> osb->uuid_str in ocfs2_setup_osb_uuid().  That's 32 characters and
NULL
> which easily fits into O2NM_MAX_NAME_LEN.  This patch doesn't change
how
> the code works, but I think it makes the code a little cleaner.
> 
> Signed-off-by: Dan Carpenter <error27 at gmail.com>
> ---
> Or we could get rid of check entirely.
> 
> diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
> index 6b5a492..084b051 100644
> --- a/fs/ocfs2/dlm/dlmdomain.c
> +++ b/fs/ocfs2/dlm/dlmdomain.c
> @@ -1671,7 +1671,7 @@ struct dlm_ctxt * dlm_register_domain(const char
*domain,
>  	struct dlm_ctxt *dlm = NULL;
>  	struct dlm_ctxt *new_ctxt = NULL;
>  
> -	if (strlen(domain) > O2NM_MAX_NAME_LEN) {
> +	if (strlen(domain) >= O2NM_MAX_NAME_LEN) {
>  		ret = -ENAMETOOLONG;
>  		mlog(ML_ERROR, "domain name length too long\n");
>  		goto leave;

Acked-by: Sunil Mushran <sunil.mushran at oracle.com>

On 07/10/2010 07:33 AM, Dan Carpenter wrote:
> This function is only called from one place and it's like this:
> 	dlm_register_domain(conn->cc_name, dlm_key,&fs_version);
>
> The "conn->cc_name" is 64 characters long.  If
strlen(conn->cc_name)
> were equal to O2NM_MAX_NAME_LEN (64) that would be a bug because
> strlen() doesn't count the NULL character.
>
> In fact, if you look how O2NM_MAX_NAME_LEN is used, it mostly describes
> 64 character buffers.  The only exception is nd_name from struct
> o2nm_node.
>
> Anyway I looked into it and in this case the domain string comes from
> osb->uuid_str in ocfs2_setup_osb_uuid().  That's 32 characters and
NULL
> which easily fits into O2NM_MAX_NAME_LEN.  This patch doesn't change
how
> the code works, but I think it makes the code a little cleaner.
>
> Signed-off-by: Dan Carpenter<error27 at gmail.com>
> ---
> Or we could get rid of check entirely.
>
> diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
> index 6b5a492..084b051 100644
> --- a/fs/ocfs2/dlm/dlmdomain.c
> +++ b/fs/ocfs2/dlm/dlmdomain.c
> @@ -1671,7 +1671,7 @@ struct dlm_ctxt * dlm_register_domain(const char
*domain,
>   	struct dlm_ctxt *dlm = NULL;
>   	struct dlm_ctxt *new_ctxt = NULL;
>
> -	if (strlen(domain)>  O2NM_MAX_NAME_LEN) {
> +	if (strlen(domain)>= O2NM_MAX_NAME_LEN) {
>   		ret = -ENAMETOOLONG;
>   		mlog(ML_ERROR, "domain name length too long\n");
>   		goto leave;
>

On Sat, Jul 10, 2010 at 04:33:36PM +0200, Dan Carpenter
wrote:
> This function is only called from one place and it's like this:
> 	dlm_register_domain(conn->cc_name, dlm_key, &fs_version);
> 
> The "conn->cc_name" is 64 characters long.  If
strlen(conn->cc_name)
> were equal to O2NM_MAX_NAME_LEN (64) that would be a bug because
> strlen() doesn't count the NULL character.
> 
> In fact, if you look how O2NM_MAX_NAME_LEN is used, it mostly describes
> 64 character buffers.  The only exception is nd_name from struct
> o2nm_node.
> 
> Anyway I looked into it and in this case the domain string comes from
> osb->uuid_str in ocfs2_setup_osb_uuid().  That's 32 characters and
NULL
> which easily fits into O2NM_MAX_NAME_LEN.  This patch doesn't change
how
> the code works, but I think it makes the code a little cleaner.
> 
> Signed-off-by: Dan Carpenter <error27 at gmail.com>
	This patch is now in the fixes branch of ocfs2.git.

Joel

-- 

Viro's Razor:
	Any race condition, no matter how unlikely, will occur just
	often enough to bite you.

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127