[patch] net/tipc: sprintf/strcpy conversion

[patch] net/tipc: sprintf/strcpy conversion

[Florian Westphal]

From: Florian Westphal <fw@strlen.de>

convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.

Signed-off-by: Florian Westphal <fw@strlen.de>

---

compile tested; diffed against davem/net-2.6.

--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
 static struct link *bcl = NULL;
 static DEFINE_SPINLOCK(bc_lock);
 
-char tipc_bclink_name[] = "multicast-link";
+const char tipc_bclink_name[] = "multicast-link";
 
 
 static u32 buf_seqno(struct sk_buff *buf)
@@ -790,7 +790,7 @@ int tipc_bclink_init(void)
 	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
 	bcbearer->bearer.media = &bcbearer->media;
 	bcbearer->media.send_msg = tipc_bcbearer_send;
-	sprintf(bcbearer->media.name, "tipc-multicast");
+	strcpy(bcbearer->media.name, "tipc-multicast");
 
 	bcl = &bclink->link;
 	memset(bclink, 0, sizeof(struct bclink));
@@ -802,7 +802,7 @@ int tipc_bclink_init(void)
 	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
 	bcl->b_ptr = &bcbearer->bearer;
 	bcl->state = WORKING_WORKING;
-	sprintf(bcl->name, tipc_bclink_name);
+	strcpy(bcl->name, tipc_bclink_name);
 
 	if (BCLINK_LOG_BUF_SIZE) {
 		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
--- a/net/tipc/bcast.h
+++ b/net/tipc/bcast.h
@@ -70,7 +70,7 @@ struct port_list {
 
 struct node;
 
-extern char tipc_bclink_name[];
+extern const char tipc_bclink_name[];
 
 
 /**
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
         link_info.dest = tipc_own_addr & 0xfffff00;
 	link_info.dest = htonl(link_info.dest);
         link_info.up = htonl(1);
-        sprintf(link_info.str, tipc_bclink_name);
+        strcpy(link_info.str, tipc_bclink_name);
 	tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
 
 	/* Add TLVs for any other links in scope */

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

hi Florian,

These line
+	strcpy(bcbearer->media.name, "tipc-multicast");

i gues that means tipc_bclink_name ?
an even more secure version could be like this:

	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);

(in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
that wchat_t will never reach the kernel)

re,
 wh




Florian Westphal wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> ---
> 
> compile tested; diffed against davem/net-2.6.
> 
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
>  static struct link *bcl = NULL;
>  static DEFINE_SPINLOCK(bc_lock);
>  
> -char tipc_bclink_name[] = "multicast-link";
> +const char tipc_bclink_name[] = "multicast-link";
>  
>  
>  static u32 buf_seqno(struct sk_buff *buf)
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
>  	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
>  	bcbearer->bearer.media = &bcbearer->media;
>  	bcbearer->media.send_msg = tipc_bcbearer_send;
> -	sprintf(bcbearer->media.name, "tipc-multicast");
> +	strcpy(bcbearer->media.name, "tipc-multicast");
>  
>  	bcl = &bclink->link;
>  	memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
>  	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
>  	bcl->b_ptr = &bcbearer->bearer;
>  	bcl->state = WORKING_WORKING;
> -	sprintf(bcl->name, tipc_bclink_name);
> +	strcpy(bcl->name, tipc_bclink_name);
>  
>  	if (BCLINK_LOG_BUF_SIZE) {
>  		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
> --- a/net/tipc/bcast.h
> +++ b/net/tipc/bcast.h
> @@ -70,7 +70,7 @@ struct port_list {
>  
>  struct node;
>  
> -extern char tipc_bclink_name[];
> +extern const char tipc_bclink_name[];
>  
>  
>  /**
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
>          link_info.dest = tipc_own_addr & 0xfffff00;
>  	link_info.dest = htonl(link_info.dest);
>          link_info.up = htonl(1);
> -        sprintf(link_info.str, tipc_bclink_name);
> +        strcpy(link_info.str, tipc_bclink_name);
>  	tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
>  
>  	/* Add TLVs for any other links in scope */
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
> 
> 
> 

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[David Miller]

From: walter harms <wharms@bfs.de>
Date: Wed, 01 Nov 2006 22:38:26 +0100

> These line
> +	strcpy(bcbearer->media.name, "tipc-multicast");
> 
> i gues that means tipc_bclink_name ?

Why?  The original code used "tipc-multicast" which is a
different string than tipc_bclink_name which is "multicast-link".

> > -	sprintf(bcbearer->media.name, "tipc-multicast");
> > +	strcpy(bcbearer->media.name, "tipc-multicast");

If you are arguing that it should be changed, that's a different
changeset to discuss.

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

David Miller wrote:
> From: walter harms <wharms@bfs.de>
> Date: Wed, 01 Nov 2006 22:38:26 +0100
> 
>> These line
>> +	strcpy(bcbearer->media.name, "tipc-multicast");
>>
>> i gues that means tipc_bclink_name ?

mea culpa, i should not write mail when tired.


> 
> Why?  The original code used "tipc-multicast" which is a
> different string than tipc_bclink_name which is "multicast-link".
> 
>>> -	sprintf(bcbearer->media.name, "tipc-multicast");
>>> +	strcpy(bcbearer->media.name, "tipc-multicast");
> 
> If you are arguing that it should be changed, that's a different
> changeset to discuss.
> 
> 
> 

Re: [tipc-discussion] [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Florian Westphal]

walter harms <wharms@bfs.de> wrote:
> These line
>+       strcpy(bcbearer->media.name, "tipc-multicast");
> i gues that means tipc_bclink_name ?

The idea was to change how things are done, not _what_ is being done.

> an even more secure version could be like this:
> 
> 	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);

Ugh, please, no. The size of src is known in all cases; there is
absoluty no point in using str(n|l)cpy here.

> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
> that wchat_t will never reach the kernel)

In this case 'someone' should be really hurt, don't you think?

Florian

Re: [KJ] [tipc-discussion] [patch] net/tipc: sprintf/strcpy conversion

[walter harms]

Florian Westphal wrote:
> walter harms <wharms@bfs.de> wrote:
>> These line
>> +       strcpy(bcbearer->media.name, "tipc-multicast");
>> i gues that means tipc_bclink_name ?
> 
> The idea was to change how things are done, not _what_ is being done.
> 
>> an even more secure version could be like this:
>>
>> 	strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
> 
> Ugh, please, no. The size of src is known in all cases; there is
> absoluty no point in using str(n|l)cpy here.
> 
>> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
>> that wchat_t will never reach the kernel)
> 
> In this case 'someone' should be really hurt, don't you think?
> 

hi florian,
i am on the side of error, the code increase is marginal and the speed penalty also, so why not ?
you make sure that an overflow may never happen, and the rest in name gets zeroed.

The problem is that when the error occurs it may be later than the actual changeset.
NTL it is an hint, and if you feel ok with it and the maintainer has no objects i have no problems either.

re,
 wh

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Alexey Dobriyan]

On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.

Ahhh, I missed the start of threads.

Patch is useless because it changes one unbounded string function into
another unbounded string function.

> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
>  	INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
>  	bcbearer->bearer.media = &bcbearer->media;
>  	bcbearer->media.send_msg = tipc_bcbearer_send;
> -	sprintf(bcbearer->media.name, "tipc-multicast");
> +	strcpy(bcbearer->media.name, "tipc-multicast");
>
>  	bcl = &bclink->link;
>  	memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
>  	tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
>  	bcl->b_ptr = &bcbearer->bearer;
>  	bcl->state = WORKING_WORKING;
> -	sprintf(bcl->name, tipc_bclink_name);
> +	strcpy(bcl->name, tipc_bclink_name);
>
>  	if (BCLINK_LOG_BUF_SIZE) {
>  		char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
>          link_info.dest = tipc_own_addr & 0xfffff00;
>  	link_info.dest = htonl(link_info.dest);
>          link_info.up = htonl(1);
> -        sprintf(link_info.str, tipc_bclink_name);
> +        strcpy(link_info.str, tipc_bclink_name);

Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion

[Hagen Paul Pfeifer]

* Alexey Dobriyan | 2006-11-03 03:09:05 [+0300]:

>On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
>> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
>Ahhh, I missed the start of threads.
>
>Patch is useless because it changes one unbounded string function into
>another unbounded string function.

The discussion in this thread is really back-breaking!

1. To make tipc_bclink_name const there is absolutly no objection
2. Replace sprintf with strcpy

  a) First of all: If you _copy_ a string then use also strCPY()
     Thats a question of good style!

  b) If the compiler is smart enough, he realize that you want to copy 
     a string and replace the sprintf call with a
        pushl   %ebx
        call    strcpy
     Surprise - Surprise!

     Assumed you use gcc with -Os or -O2! Don't know how icc handle this 
     case. If you compile without optimization you save at least a
     "repz movsb %ds:(%esi),%es:(%edi)" instruction.

   c) Last but not least I read all the time this patch doesn't introduce
      bounds-checking. This isn't a argument because the author is aware of
      the destination length of the buffer. BTW: grep for (sprintf|strcpy) in
      /usr/src/linux and be surprised how unsecure the kernel is (thats
      ironical).

This patch is 100% OK!

HGN