[PATCH] drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

[PATCH] drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

[Chen Gang]

  The fields must be null-terminated, or IPW_DEBUG_ASSOC will cause issue.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 drivers/net/wireless/ipw2x00/ipw2200.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
index 844f201..c85261b 100644
--- a/drivers/net/wireless/ipw2x00/ipw2200.c
+++ b/drivers/net/wireless/ipw2x00/ipw2200.c
@@ -5558,7 +5558,7 @@ static int ipw_find_adhoc_network(struct ipw_priv *priv,
 			    min(network->ssid_len, priv->essid_len)))) {
 			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
 
-			strncpy(escaped,
+			strlcpy(escaped,
 				print_ssid(ssid, network->ssid,
 					   network->ssid_len),
 				sizeof(escaped));
@@ -5771,7 +5771,7 @@ static int ipw_best_network(struct ipw_priv *priv,
 		     memcmp(network->ssid, priv->essid,
 			    min(network->ssid_len, priv->essid_len)))) {
 			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
-			strncpy(escaped,
+			strlcpy(escaped,
 				print_ssid(ssid, network->ssid,
 					   network->ssid_len),
 				sizeof(escaped));
@@ -5788,7 +5788,7 @@ static int ipw_best_network(struct ipw_priv *priv,
 	 * testing everything else. */
 	if (match->network && match->network->stats.rssi > network->stats.rssi) {
 		char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
-		strncpy(escaped,
+		strlcpy(escaped,
 			print_ssid(ssid, network->ssid, network->ssid_len),
 			sizeof(escaped));
 		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
-- 
1.7.10.4

Re: [PATCH] drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

[Joe Perches]

On Sat, 2013-01-05 at 21:41 +0800, Chen Gang wrote:
>   The fields must be null-terminated, or IPW_DEBUG_ASSOC will cause issue.
[]
> diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
[]
> @@ -5558,7 +5558,7 @@ static int ipw_find_adhoc_network(struct ipw_priv *priv,
>  			    min(network->ssid_len, priv->essid_len)))) {
>  			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
>  
> -			strncpy(escaped,
> +			strlcpy(escaped,
>  				print_ssid(ssid, network->ssid,
>  					   network->ssid_len),
>  				sizeof(escaped));
> @@ -5771,7 +5771,7 @@ static int ipw_best_network(struct ipw_priv *priv,
>  		     memcmp(network->ssid, priv->essid,
>  			    min(network->ssid_len, priv->essid_len)))) {
>  			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> -			strncpy(escaped,
> +			strlcpy(escaped,
>  				print_ssid(ssid, network->ssid,
>  					   network->ssid_len),
>  				sizeof(escaped));
> @@ -5788,7 +5788,7 @@ static int ipw_best_network(struct ipw_priv *priv,
>  	 * testing everything else. */
>  	if (match->network && match->network->stats.rssi > network->stats.rssi) {
>  		char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> -		strncpy(escaped,
> +		strlcpy(escaped,
>  			print_ssid(ssid, network->ssid, network->ssid_len),
>  			sizeof(escaped));
>  		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "

This happens because escaped is declared the wrong size.

It'd be better to change
	char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
to
	DECLARE_SSID_BUF(escaped);
and use
	print_ssid(escaped, network->ssid, network->ssid_len)
in the debug.

Re: [PATCH] drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

[Chen Gang]

于 2013年01月05日 22:42, Joe Perches 写道:
> This happens because escaped is declared the wrong size.
> 
> It'd be better to change
> 	char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> to
> 	DECLARE_SSID_BUF(escaped);
> and use
> 	print_ssid(escaped, network->ssid, network->ssid_len)
> in the debug.
> 

  if what you said is true:
    it is better to delete escaped variable
    use ssid instead of escaped, directly.

  but I think the original author intended to use escaped instead of ssid
    DECLARE_SSID_BUF(ssid)  (line 5525, 5737)
    use ssid to print debug information directly
      (such as: line 5530..5535, 5545..5549, 5745..5749, ...)
    when need print additional information, use escaped
      (line 5559..5569, 5773..5782, 5791..5799)

  so, I still suggest:
    only fix the bug (use strlcpy instead of strncpy)
    and not touch original features which orignal author intended using.

  Regards

gchen.

in drivers/net/wireless/ipw2x00/ipw2200.c:

 5519 static int ipw_find_adhoc_network(struct ipw_priv *priv,
 5520                                   struct ipw_network_match *match,
 5521                                   struct libipw_network *network,
 5522                                   int roaming)
 5523 {
 5524         struct ipw_supported_rates rates;
 5525         DECLARE_SSID_BUF(ssid);
 5526 
 5527         /* Verify that this network's capability is compatible with the
 5528          * current mode (AdHoc or Infrastructure) */
 5529         if ((priv->ieee->iw_mode == IW_MODE_ADHOC &&
 5530              !(network->capability & WLAN_CAPABILITY_IBSS))) {
 5531                 IPW_DEBUG_MERGE("Network '%s (%pM)' excluded due to "
 5532                                 "capability mismatch.\n",
 5533                                 print_ssid(ssid, network->ssid,
 5534                                            network->ssid_len),
 5535                                 network->bssid);
 5536                 return 0;
 5537         }
 5538 
 5539         if (unlikely(roaming)) {
 5540                 /* If we are roaming, then ensure check if this is a valid
 5541                  * network to try and roam to */
 5542                 if ((network->ssid_len != match->network->ssid_len) ||
 5543                     memcmp(network->ssid, match->network->ssid,
 5544                            network->ssid_len)) {
 5545                         IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
 5546                                         "because of non-network ESSID.\n",
 5547                                         print_ssid(ssid, network->ssid,
 5548                                                    network->ssid_len),
 5549                                         network->bssid);
 5550                         return 0;
 5551                 }
 5552         } else {
 5553                 /* If an ESSID has been configured then compare the broadcast
 5554                  * ESSID to ours */
 5555                 if ((priv->config & CFG_STATIC_ESSID) &&
 5556                     ((network->ssid_len != priv->essid_len) ||
 5557                      memcmp(network->ssid, priv->essid,
 5558                             min(network->ssid_len, priv->essid_len)))) {
 5559                         char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
 5560 
 5561                         strncpy(escaped,
 5562                                 print_ssid(ssid, network->ssid,
 5563                                            network->ssid_len),
 5564                                 sizeof(escaped));
 5565                         IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
 5566                                         "because of ESSID mismatch: '%s'.\n",
 5567                                         escaped, network->bssid,
 5568                                         print_ssid(ssid, priv->essid,
 5569                                                    priv->essid_len));
 5570                         return 0;
 5571                 }
 5572         }
 ...
 
 5732 static int ipw_best_network(struct ipw_priv *priv,
 5733                             struct ipw_network_match *match,
 5734                             struct libipw_network *network, int roaming)
 5735 {
 5736         struct ipw_supported_rates rates;
 5737         DECLARE_SSID_BUF(ssid);
 5738 
 5739         /* Verify that this network's capability is compatible with the
 5740          * current mode (AdHoc or Infrastructure) */
 5741         if ((priv->ieee->iw_mode == IW_MODE_INFRA &&
 5742              !(network->capability & WLAN_CAPABILITY_ESS)) ||
 5743             (priv->ieee->iw_mode == IW_MODE_ADHOC &&
 5744              !(network->capability & WLAN_CAPABILITY_IBSS))) {
 5745                 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded due to "
 5746                                 "capability mismatch.\n",
 5747                                 print_ssid(ssid, network->ssid,
 5748                                            network->ssid_len),
 5749                                 network->bssid);
 5750                 return 0;
 5751         }
 5752 
 5753         if (unlikely(roaming)) {
 5754                 /* If we are roaming, then ensure check if this is a valid
 5755                  * network to try and roam to */
 5756                 if ((network->ssid_len != match->network->ssid_len) ||
 5757                     memcmp(network->ssid, match->network->ssid,
 5758                            network->ssid_len)) {
 5759                         IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
 5760                                         "because of non-network ESSID.\n",
 5761                                         print_ssid(ssid, network->ssid,
 5762                                                    network->ssid_len),
 5763                                         network->bssid);
 5764                         return 0;
 5765                 }
 5766         } else {
 5767                 /* If an ESSID has been configured then compare the broadcast
 5768                  * ESSID to ours */
 5769                 if ((priv->config & CFG_STATIC_ESSID) &&
 5770                     ((network->ssid_len != priv->essid_len) ||
 5771                      memcmp(network->ssid, priv->essid,
 5772                             min(network->ssid_len, priv->essid_len)))) {
 5773                         char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
 5774                         strncpy(escaped,
 5775                                 print_ssid(ssid, network->ssid,
 5776                                            network->ssid_len),
 5777                                 sizeof(escaped));
 5778                         IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
 5779                                         "because of ESSID mismatch: '%s'.\n",
 5780                                         escaped, network->bssid,
 5781                                         print_ssid(ssid, priv->essid,
 5782                                                    priv->essid_len));
 5783                         return 0;
 5784                 }
 5785         }
 5786 
 5787         /* If the old network rate is better than this one, don't bother
 5788          * testing everything else. */
 5789         if (match->network && match->network->stats.rssi > network->stats.rssi) {
 5790                 char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
 5791                 strncpy(escaped,
 5792                         print_ssid(ssid, network->ssid, network->ssid_len),
 5793                         sizeof(escaped));
 5794                 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
 5795                                 "'%s (%pM)' has a stronger signal.\n",
 5796                                 escaped, network->bssid,
 5797                                 print_ssid(ssid, match->network->ssid,
 5798                                            match->network->ssid_len),
 5799                                 match->network->bssid);
 5800                 return 0;
 5801         }

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

[Chen Gang F T]

[-- Attachment #1: Type: text/plain, Size: 8770 bytes --]

于 2013年01月07日 10:49, Chen Gang 写道:
> 于 2013年01月05日 22:42, Joe Perches 写道:
>> This happens because escaped is declared the wrong size.
>>
>> It'd be better to change
>> 	char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
>> to
>> 	DECLARE_SSID_BUF(escaped);
>> and use
>> 	print_ssid(escaped, network->ssid, network->ssid_len)
>> in the debug.
>>
> 
>   if what you said is true:
>     it is better to delete escaped variable
>     use ssid instead of escaped, directly.
> 
   oh, sorry, it is my fault.
   we need use duplicate buffer to print different contents, at the same time.

   :-)

   but I still suggest to keep original author using
      maybe he intend to keep the print size for output format
      so I think it is better to only fix bug, not touch the features. 

   Regards

 gchen.



>   but I think the original author intended to use escaped instead of ssid
>     DECLARE_SSID_BUF(ssid)  (line 5525, 5737)
>     use ssid to print debug information directly
>       (such as: line 5530..5535, 5545..5549, 5745..5749, ...)
>     when need print additional information, use escaped
>       (line 5559..5569, 5773..5782, 5791..5799)
> 
>   so, I still suggest:
>     only fix the bug (use strlcpy instead of strncpy)
>     and not touch original features which orignal author intended using.
> 
>   Regards
> 
> gchen.
> 
> in drivers/net/wireless/ipw2x00/ipw2200.c:
> 
>  5519 static int ipw_find_adhoc_network(struct ipw_priv *priv,
>  5520                                   struct ipw_network_match *match,
>  5521                                   struct libipw_network *network,
>  5522                                   int roaming)
>  5523 {
>  5524         struct ipw_supported_rates rates;
>  5525         DECLARE_SSID_BUF(ssid);
>  5526 
>  5527         /* Verify that this network's capability is compatible with the
>  5528          * current mode (AdHoc or Infrastructure) */
>  5529         if ((priv->ieee->iw_mode == IW_MODE_ADHOC &&
>  5530              !(network->capability & WLAN_CAPABILITY_IBSS))) {
>  5531                 IPW_DEBUG_MERGE("Network '%s (%pM)' excluded due to "
>  5532                                 "capability mismatch.\n",
>  5533                                 print_ssid(ssid, network->ssid,
>  5534                                            network->ssid_len),
>  5535                                 network->bssid);
>  5536                 return 0;
>  5537         }
>  5538 
>  5539         if (unlikely(roaming)) {
>  5540                 /* If we are roaming, then ensure check if this is a valid
>  5541                  * network to try and roam to */
>  5542                 if ((network->ssid_len != match->network->ssid_len) ||
>  5543                     memcmp(network->ssid, match->network->ssid,
>  5544                            network->ssid_len)) {
>  5545                         IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
>  5546                                         "because of non-network ESSID.\n",
>  5547                                         print_ssid(ssid, network->ssid,
>  5548                                                    network->ssid_len),
>  5549                                         network->bssid);
>  5550                         return 0;
>  5551                 }
>  5552         } else {
>  5553                 /* If an ESSID has been configured then compare the broadcast
>  5554                  * ESSID to ours */
>  5555                 if ((priv->config & CFG_STATIC_ESSID) &&
>  5556                     ((network->ssid_len != priv->essid_len) ||
>  5557                      memcmp(network->ssid, priv->essid,
>  5558                             min(network->ssid_len, priv->essid_len)))) {
>  5559                         char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
>  5560 
>  5561                         strncpy(escaped,
>  5562                                 print_ssid(ssid, network->ssid,
>  5563                                            network->ssid_len),
>  5564                                 sizeof(escaped));
>  5565                         IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
>  5566                                         "because of ESSID mismatch: '%s'.\n",
>  5567                                         escaped, network->bssid,
>  5568                                         print_ssid(ssid, priv->essid,
>  5569                                                    priv->essid_len));
>  5570                         return 0;
>  5571                 }
>  5572         }
>  ...
>  
>  5732 static int ipw_best_network(struct ipw_priv *priv,
>  5733                             struct ipw_network_match *match,
>  5734                             struct libipw_network *network, int roaming)
>  5735 {
>  5736         struct ipw_supported_rates rates;
>  5737         DECLARE_SSID_BUF(ssid);
>  5738 
>  5739         /* Verify that this network's capability is compatible with the
>  5740          * current mode (AdHoc or Infrastructure) */
>  5741         if ((priv->ieee->iw_mode == IW_MODE_INFRA &&
>  5742              !(network->capability & WLAN_CAPABILITY_ESS)) ||
>  5743             (priv->ieee->iw_mode == IW_MODE_ADHOC &&
>  5744              !(network->capability & WLAN_CAPABILITY_IBSS))) {
>  5745                 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded due to "
>  5746                                 "capability mismatch.\n",
>  5747                                 print_ssid(ssid, network->ssid,
>  5748                                            network->ssid_len),
>  5749                                 network->bssid);
>  5750                 return 0;
>  5751         }
>  5752 
>  5753         if (unlikely(roaming)) {
>  5754                 /* If we are roaming, then ensure check if this is a valid
>  5755                  * network to try and roam to */
>  5756                 if ((network->ssid_len != match->network->ssid_len) ||
>  5757                     memcmp(network->ssid, match->network->ssid,
>  5758                            network->ssid_len)) {
>  5759                         IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
>  5760                                         "because of non-network ESSID.\n",
>  5761                                         print_ssid(ssid, network->ssid,
>  5762                                                    network->ssid_len),
>  5763                                         network->bssid);
>  5764                         return 0;
>  5765                 }
>  5766         } else {
>  5767                 /* If an ESSID has been configured then compare the broadcast
>  5768                  * ESSID to ours */
>  5769                 if ((priv->config & CFG_STATIC_ESSID) &&
>  5770                     ((network->ssid_len != priv->essid_len) ||
>  5771                      memcmp(network->ssid, priv->essid,
>  5772                             min(network->ssid_len, priv->essid_len)))) {
>  5773                         char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
>  5774                         strncpy(escaped,
>  5775                                 print_ssid(ssid, network->ssid,
>  5776                                            network->ssid_len),
>  5777                                 sizeof(escaped));
>  5778                         IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
>  5779                                         "because of ESSID mismatch: '%s'.\n",
>  5780                                         escaped, network->bssid,
>  5781                                         print_ssid(ssid, priv->essid,
>  5782                                                    priv->essid_len));
>  5783                         return 0;
>  5784                 }
>  5785         }
>  5786 
>  5787         /* If the old network rate is better than this one, don't bother
>  5788          * testing everything else. */
>  5789         if (match->network && match->network->stats.rssi > network->stats.rssi) {
>  5790                 char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
>  5791                 strncpy(escaped,
>  5792                         print_ssid(ssid, network->ssid, network->ssid_len),
>  5793                         sizeof(escaped));
>  5794                 IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
>  5795                                 "'%s (%pM)' has a stronger signal.\n",
>  5796                                 escaped, network->bssid,
>  5797                                 print_ssid(ssid, match->network->ssid,
>  5798                                            match->network->ssid_len),
>  5799                                 match->network->bssid);
>  5800                 return 0;
>  5801         }
> 


-- 
Chen Gang

Flying Transformer

[-- Attachment #2: chen_gang_flying_transformer.vcf --]
[-- Type: text/x-vcard, Size: 67 bytes --]

begin:vcard
fn:Chen Gang
n:;Chen Gang
version:2.1
end:vcard

Re: [PATCH] drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

[Joe Perches]

On Mon, 2013-01-07 at 10:49 +0800, Chen Gang wrote:
>   but I think the original author intended to use escaped instead of ssid
>     DECLARE_SSID_BUF(ssid)  (line 5525, 5737)
>     use ssid to print debug information directly
>       (such as: line 5530..5535, 5545..5549, 5745..5749, ...)
>     when need print additional information, use escaped
>       (line 5559..5569, 5773..5782, 5791..5799)
> 
>   so, I still suggest:
>     only fix the bug (use strlcpy instead of strncpy)
>     and not touch original features which orignal author intended using.

More likely John Linville just missed the conversions.
4+ years ago.

commit 9387b7caf3049168fc97a8a9111af8fe2143af18
Author: John W. Linville <linville@tuxdriver.com>
Date:   Tue Sep 30 20:59:05 2008 -0400

    wireless: use individual buffers for printing ssid values
  
    Also change escape_ssid to print_ssid to match print_mac semantics.
    
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

Maybe these days this should be another vsprintf %p extension
like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.

(or maybe extend %ph for ssids with %*phs, length, array)

But if and until then, I suggest this instead:

 drivers/net/wireless/ipw2x00/ipw2200.c | 38 ++++++++++++++--------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
index 844f201..3dc6a92 100644
--- a/drivers/net/wireless/ipw2x00/ipw2200.c
+++ b/drivers/net/wireless/ipw2x00/ipw2200.c
@@ -5556,15 +5556,12 @@ static int ipw_find_adhoc_network(struct ipw_priv *priv,
 		    ((network->ssid_len != priv->essid_len) ||
 		     memcmp(network->ssid, priv->essid,
 			    min(network->ssid_len, priv->essid_len)))) {
-			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
+			DECLARE_SSID_BUF(escaped);
 
-			strncpy(escaped,
-				print_ssid(ssid, network->ssid,
-					   network->ssid_len),
-				sizeof(escaped));
-			IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
-					"because of ESSID mismatch: '%s'.\n",
-					escaped, network->bssid,
+			IPW_DEBUG_MERGE("Network '%s (%pM)' excluded because of ESSID mismatch: '%s'\n",
+					print_ssid(escaped, network->ssid,
+						   network->ssid_len),
+					network->bssid,
 					print_ssid(ssid, priv->essid,
 						   priv->essid_len));
 			return 0;
@@ -5770,14 +5767,11 @@ static int ipw_best_network(struct ipw_priv *priv,
 		    ((network->ssid_len != priv->essid_len) ||
 		     memcmp(network->ssid, priv->essid,
 			    min(network->ssid_len, priv->essid_len)))) {
-			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
-			strncpy(escaped,
-				print_ssid(ssid, network->ssid,
-					   network->ssid_len),
-				sizeof(escaped));
-			IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
-					"because of ESSID mismatch: '%s'.\n",
-					escaped, network->bssid,
+			DECLARE_SSID_BUF(escaped);
+			IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because of ESSID mismatch: '%s'\n",
+					print_ssid(escaped, network->ssid,
+						   network->ssid_len),
+					network->bssid,
 					print_ssid(ssid, priv->essid,
 						   priv->essid_len));
 			return 0;
@@ -5787,13 +5781,11 @@ static int ipw_best_network(struct ipw_priv *priv,
 	/* If the old network rate is better than this one, don't bother
 	 * testing everything else. */
 	if (match->network && match->network->stats.rssi > network->stats.rssi) {
-		char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
-		strncpy(escaped,
-			print_ssid(ssid, network->ssid, network->ssid_len),
-			sizeof(escaped));
-		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
-				"'%s (%pM)' has a stronger signal.\n",
-				escaped, network->bssid,
+		DECLARE_SSID_BUF(escaped);
+		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because '%s (%pM)' has a stronger signal\n",
+				print_ssid(escaped, network->ssid,
+					   network->ssid_len),
+				network->bssid,
 				print_ssid(ssid, match->network->ssid,
 					   match->network->ssid_len),
 				match->network->bssid);

Re: [PATCH] drivers/net/wireless/ipw2x00: use strlcpy instead of strncpy

[Chen Gang F T]

[-- Attachment #1: Type: text/plain, Size: 5228 bytes --]

于 2013年01月07日 11:19, Joe Perches 写道:
> On Mon, 2013-01-07 at 10:49 +0800, Chen Gang wrote:
>>   but I think the original author intended to use escaped instead of ssid
>>     DECLARE_SSID_BUF(ssid)  (line 5525, 5737)
>>     use ssid to print debug information directly
>>       (such as: line 5530..5535, 5545..5549, 5745..5749, ...)
>>     when need print additional information, use escaped
>>       (line 5559..5569, 5773..5782, 5791..5799)
>>
>>   so, I still suggest:
>>     only fix the bug (use strlcpy instead of strncpy)
>>     and not touch original features which orignal author intended using.
> 
> More likely John Linville just missed the conversions.
> 4+ years ago.
> 

  I wonder why it is not integrated into main branch.
    maybe we miss it.
      if so, I suggest to integrate it (better also add Reported-by gang.chen@asianux.com  :-) )
    maybe original author intended using short length.
      if so, I suggest to integrate my patch (using strlcpy instead of strncpy).

  for me:
    using long size instead of original short size, will change the output format.
    it seems not a good idea to change the original output format.
      (especially, the original output format has existence 4+ years).

  so, at least:
    for only fixing bug, not touching original features
      it is an executable method (is a valuable patch).
      although maybe it is not a best method (is not a very good patch).

 Regards

gchen.

> commit 9387b7caf3049168fc97a8a9111af8fe2143af18
> Author: John W. Linville <linville@tuxdriver.com>
> Date:   Tue Sep 30 20:59:05 2008 -0400
> 
>     wireless: use individual buffers for printing ssid values
>   
>     Also change escape_ssid to print_ssid to match print_mac semantics.
>     
>     Signed-off-by: John W. Linville <linville@tuxdriver.com>
> 
> Maybe these days this should be another vsprintf %p extension
> like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> 
> (or maybe extend %ph for ssids with %*phs, length, array)
> 
> But if and until then, I suggest this instead:
> 
>  drivers/net/wireless/ipw2x00/ipw2200.c | 38 ++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
> index 844f201..3dc6a92 100644
> --- a/drivers/net/wireless/ipw2x00/ipw2200.c
> +++ b/drivers/net/wireless/ipw2x00/ipw2200.c
> @@ -5556,15 +5556,12 @@ static int ipw_find_adhoc_network(struct ipw_priv *priv,
>  		    ((network->ssid_len != priv->essid_len) ||
>  		     memcmp(network->ssid, priv->essid,
>  			    min(network->ssid_len, priv->essid_len)))) {
> -			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> +			DECLARE_SSID_BUF(escaped);
>  
> -			strncpy(escaped,
> -				print_ssid(ssid, network->ssid,
> -					   network->ssid_len),
> -				sizeof(escaped));
> -			IPW_DEBUG_MERGE("Network '%s (%pM)' excluded "
> -					"because of ESSID mismatch: '%s'.\n",
> -					escaped, network->bssid,
> +			IPW_DEBUG_MERGE("Network '%s (%pM)' excluded because of ESSID mismatch: '%s'\n",
> +					print_ssid(escaped, network->ssid,
> +						   network->ssid_len),
> +					network->bssid,
>  					print_ssid(ssid, priv->essid,
>  						   priv->essid_len));
>  			return 0;
> @@ -5770,14 +5767,11 @@ static int ipw_best_network(struct ipw_priv *priv,
>  		    ((network->ssid_len != priv->essid_len) ||
>  		     memcmp(network->ssid, priv->essid,
>  			    min(network->ssid_len, priv->essid_len)))) {
> -			char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> -			strncpy(escaped,
> -				print_ssid(ssid, network->ssid,
> -					   network->ssid_len),
> -				sizeof(escaped));
> -			IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded "
> -					"because of ESSID mismatch: '%s'.\n",
> -					escaped, network->bssid,
> +			DECLARE_SSID_BUF(escaped);
> +			IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because of ESSID mismatch: '%s'\n",
> +					print_ssid(escaped, network->ssid,
> +						   network->ssid_len),
> +					network->bssid,
>  					print_ssid(ssid, priv->essid,
>  						   priv->essid_len));
>  			return 0;
> @@ -5787,13 +5781,11 @@ static int ipw_best_network(struct ipw_priv *priv,
>  	/* If the old network rate is better than this one, don't bother
>  	 * testing everything else. */
>  	if (match->network && match->network->stats.rssi > network->stats.rssi) {
> -		char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> -		strncpy(escaped,
> -			print_ssid(ssid, network->ssid, network->ssid_len),
> -			sizeof(escaped));
> -		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because "
> -				"'%s (%pM)' has a stronger signal.\n",
> -				escaped, network->bssid,
> +		DECLARE_SSID_BUF(escaped);
> +		IPW_DEBUG_ASSOC("Network '%s (%pM)' excluded because '%s (%pM)' has a stronger signal\n",
> +				print_ssid(escaped, network->ssid,
> +					   network->ssid_len),
> +				network->bssid,
>  				print_ssid(ssid, match->network->ssid,
>  					   match->network->ssid_len),
>  				match->network->bssid);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Chen Gang

Flying Transformer

[-- Attachment #2: chen_gang_flying_transformer.vcf --]
[-- Type: text/x-vcard, Size: 67 bytes --]

begin:vcard
fn:Chen Gang
n:;Chen Gang
version:2.1
end:vcard

[RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Joe Perches]

On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> Maybe these days this should be another vsprintf %p extension
> like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> 
> (or maybe extend %ph for ssids with %*phs, length, array)

Maybe like this:

 lib/vsprintf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index fab33a9..98916a0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -26,6 +26,7 @@
 #include <linux/math64.h>
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
+#include <linux/ieee80211.h>
 #include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
@@ -660,10 +661,59 @@ char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static noinline_for_stack
+char *ssid_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+		  const char *fmt)
+{
+	int i, len = 1;		/* if we pass %*p, field width remains
+				   negative value, fallback to the default */
+
+	if (spec.field_width == 0)
+		/* nothing to print */
+		return buf;
+
+	if (ZERO_OR_NULL_PTR(addr))
+		/* NULL pointer */
+		return string(buf, end, NULL, spec);
+
+	if (spec.field_width > 0)
+		len = min_t(int, spec.field_width, IEEE80211_MAX_SSID_LEN);
+
+	for (i = 0; i < len && buf < end; i++) {
+		if (isprint(addr[i])) {
+			*buf++ = addr[i];
+			continue;
+		}
+		*buf++ = '\\';
+		if (buf >= end)
+			continue;
+		if (addr[i] == '\0')
+			*buf++ = '0';
+		else if (addr[i] == '\n')
+			*buf++ = 'n';
+		else if (addr[i] == '\r')
+			*buf++ = 'r';
+		else if (addr[i] == '\t')
+			*buf++ = 't';
+		else if (addr[i] == '\\')
+			*buf++ = '\\';
+		else {
+			if (buf < end)
+				*buf++ = ((addr[i] >> 6) & 7) + '0';
+			if (buf < end)
+				*buf++ = ((addr[i] >> 3) & 7) + '0';
+			if (buf < end)
+				*buf++ = ((addr[i] >> 0) & 7) + '0';
+		}
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		 const char *fmt)
 {
-	int i, len = 1;		/* if we pass '%ph[CDN]', field witdh remains
+	int i, len = 1;		/* if we pass '%ph[CDN]', field width remains
 				   negative value, fallback to the default */
 	char separator;
 
@@ -695,7 +745,6 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 
 	for (i = 0; i < len && buf < end - 1; i++) {
 		buf = hex_byte_pack(buf, addr[i]);
-
 		if (buf < end && separator && i != len - 1)
 			*buf++ = separator;
 	}
@@ -1016,6 +1065,8 @@ int kptr_restrict __read_mostly;
  *             [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
  *           little endian output byte order is:
  *             [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
+ * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
+         characters with a leading \ and octal or [0nrt\]
  * - 'V' For a struct va_format which contains a format string * and va_list *,
  *       call vsnprintf(->format, *->va_list).
  *       Implements a "recursive vsnprintf".
@@ -1088,6 +1139,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		break;
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
+	case 'D':
+		return ssid_string(buf, end, ptr, spec, fmt);
 	case 'V':
 		{
 			va_list va;

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Chen Gang]

于 2013年01月07日 12:49, Joe Perches 写道:
> On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
>> Maybe these days this should be another vsprintf %p extension
>> like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
>>
>> (or maybe extend %ph for ssids with %*phs, length, array)
> 

  excuse me:
    I do not quite know how to reply the [RFC PATCH].
    it would be better if you can tell me how to do for [RFC PATCH], thanks.

    :-)


  at least for me:

    although it is good idea to add common, widely used features in public tools function.

    after searching the relative source code:
      it seems print ssid is not quite widely used (although it is a common feature).
        it is used by drivers/net/wireless/libertas
        it is used by drivers/net/wireless/ipw2x00
        no additional using in current kernel source code wide.
      if another modules want to print ssid,
        they can still call print_ssid now (EXPORT_SYMBOL in net/wireless).

    so at least now, it is not necessary to add this feature to public tools function.

  Regards

gchen.

> Maybe like this:
> 
>  lib/vsprintf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index fab33a9..98916a0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -26,6 +26,7 @@
>  #include <linux/math64.h>
>  #include <linux/uaccess.h>
>  #include <linux/ioport.h>
> +#include <linux/ieee80211.h>
>  #include <net/addrconf.h>
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
> @@ -660,10 +661,59 @@ char *resource_string(char *buf, char *end, struct resource *res,
>  }
>  
>  static noinline_for_stack
> +char *ssid_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> +		  const char *fmt)
> +{
> +	int i, len = 1;		/* if we pass %*p, field width remains
> +				   negative value, fallback to the default */
> +
> +	if (spec.field_width == 0)
> +		/* nothing to print */
> +		return buf;
> +
> +	if (ZERO_OR_NULL_PTR(addr))
> +		/* NULL pointer */
> +		return string(buf, end, NULL, spec);
> +
> +	if (spec.field_width > 0)
> +		len = min_t(int, spec.field_width, IEEE80211_MAX_SSID_LEN);
> +
> +	for (i = 0; i < len && buf < end; i++) {
> +		if (isprint(addr[i])) {
> +			*buf++ = addr[i];
> +			continue;
> +		}
> +		*buf++ = '\\';
> +		if (buf >= end)
> +			continue;
> +		if (addr[i] == '\0')
> +			*buf++ = '0';
> +		else if (addr[i] == '\n')
> +			*buf++ = 'n';
> +		else if (addr[i] == '\r')
> +			*buf++ = 'r';
> +		else if (addr[i] == '\t')
> +			*buf++ = 't';
> +		else if (addr[i] == '\\')
> +			*buf++ = '\\';
> +		else {
> +			if (buf < end)
> +				*buf++ = ((addr[i] >> 6) & 7) + '0';
> +			if (buf < end)
> +				*buf++ = ((addr[i] >> 3) & 7) + '0';
> +			if (buf < end)
> +				*buf++ = ((addr[i] >> 0) & 7) + '0';
> +		}
> +	}
> +
> +	return buf;
> +}
> +
> +static noinline_for_stack
>  char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  		 const char *fmt)
>  {
> -	int i, len = 1;		/* if we pass '%ph[CDN]', field witdh remains
> +	int i, len = 1;		/* if we pass '%ph[CDN]', field width remains
>  				   negative value, fallback to the default */
>  	char separator;
>  
> @@ -695,7 +745,6 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  
>  	for (i = 0; i < len && buf < end - 1; i++) {
>  		buf = hex_byte_pack(buf, addr[i]);
> -
>  		if (buf < end && separator && i != len - 1)
>  			*buf++ = separator;
>  	}
> @@ -1016,6 +1065,8 @@ int kptr_restrict __read_mostly;
>   *             [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
>   *           little endian output byte order is:
>   *             [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
> + * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
> +         characters with a leading \ and octal or [0nrt\]
>   * - 'V' For a struct va_format which contains a format string * and va_list *,
>   *       call vsnprintf(->format, *->va_list).
>   *       Implements a "recursive vsnprintf".
> @@ -1088,6 +1139,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		break;
>  	case 'U':
>  		return uuid_string(buf, end, ptr, spec, fmt);
> +	case 'D':
> +		return ssid_string(buf, end, ptr, spec, fmt);
>  	case 'V':
>  		{
>  			va_list va;
> 
> 
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Joe Perches]

On Mon, 2013-01-07 at 14:07 +0800, Chen Gang wrote:
> 于 2013年01月07日 12:49, Joe Perches 写道:
> > On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> >> Maybe these days this should be another vsprintf %p extension
> >> like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> >>
> >> (or maybe extend %ph for ssids with %*phs, length, array)
> > 
>   excuse me:
>     I do not quite know how to reply the [RFC PATCH].
>     it would be better if you can tell me how to do for [RFC PATCH], thanks.

You did fine except you unnecessarily quoted the entire original email.
Remember to trim your replies please.  _lots_ of people read these
mailing lists and unnecessary quoting wastes all of their times.

>   at least for me:
>     although it is good idea to add common, widely used features in public tools function.

It's akin to most of the minor uuid/bluetooth mac, etc codes
in vsprintf I've added.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Chen Gang]

于 2013年01月07日 14:37, Joe Perches 写道:
> You did fine except you unnecessarily quoted the entire original email.
> Remember to trim your replies please.  _lots_ of people read these
> mailing lists and unnecessary quoting wastes all of their times.
> 
  ok, thank you.

  I should notice, next time.  :-)

>> >   at least for me:
>> >     although it is good idea to add common, widely used features in public tools function.
> It's akin to most of the minor uuid/bluetooth mac, etc codes

  after see the comments for function pointer in lib/vsprintf.c
  sorry for I do not think it is suitable to add ssid in vsprintf
    all formate features in vsprintf are face to kernel wide, not for one sub system
      one exception is for mac and ip:
        they have already been well known in kernel wide
        every one knows about mac and ip.
    I feel, our ssid is not quite well known in kernel wide

  maybe current place (net/wireless/) is not quite suitable for print_ssid:
    for bluetooth, it is not belong to net/wireless.
    if necessary, I suggest to reconstruct it within net sub system wide, not in kernel wide.


  (it is also my fault for my original reply, it is not quite clear)


  Regards

-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Johannes Berg]

On Sun, 2013-01-06 at 20:49 -0800, Joe Perches wrote:
> On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> > Maybe these days this should be another vsprintf %p extension
> > like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> > 
> > (or maybe extend %ph for ssids with %*phs, length, array)
> 
> Maybe like this:

> + * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
> +         characters with a leading \ and octal or [0nrt\]

Honestly, I'm not sure it's worth it. print_ssid() is used in two or
three legacy drivers only, not in any modern driver, and is unlikely to
be used in the more modern drivers due to tracing etc.

johannes

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Joe Perches]

On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
> On Sun, 2013-01-06 at 20:49 -0800, Joe Perches wrote:
> > On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> > > Maybe these days this should be another vsprintf %p extension
> > > like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> > > 
> > > (or maybe extend %ph for ssids with %*phs, length, array)
> > 
> > Maybe like this:
> 
> > + * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
> > +         characters with a leading \ and octal or [0nrt\]
> 
> Honestly, I'm not sure it's worth it.

Neither am I really, the only value I see in it is the
reduction in stack consumption by 100 bytes or so per use.

> print_ssid() is used in two or
> three legacy drivers only, not in any modern driver, and is unlikely to
> be used in the more modern drivers due to tracing etc.

Swell.  It was just another way to correct those overrun
errors Chen Gang found.

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Chen Gang]

于 2013年01月08日 01:22, Joe Perches 写道:
> On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
>> > print_ssid() is used in two or
>> > three legacy drivers only, not in any modern driver, and is unlikely to
>> > be used in the more modern drivers due to tracing etc.
> Swell.  It was just another way to correct those overrun
> errors Chen Gang found.
> 

  sorry, I am not quite clear about what you said.


-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Joe Perches]

On Tue, 2013-01-08 at 10:57 +0800, Chen Gang wrote:
> 于 2013年01月08日 01:22, Joe Perches 写道:
> > On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
> >> > print_ssid() is used in two or
> >> > three legacy drivers only, not in any modern driver, and is unlikely to
> >> > be used in the more modern drivers due to tracing etc.
> > Swell.  It was just another way to correct those overrun
> > errors Chen Gang found.
> > 
>   sorry, I am not quite clear about what you said.

You found some overrun errors and proposed a patch.
Your solution could output incomplete SSIDs.

I proposed a different patch that would fully output
any binary/non-ascii printable SSID.

I also proposed a different mechanism to avoid the
overrun via printk and also avoid possibly excessive
stack consumption as a means for John Linville to
select 1 of the above options.

I don't care what is done with any of those proposed
patches.

Clear?

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] vsprintf: Add %p*D extension for 80211 SSIDs

[Chen Gang]

于 2013年01月08日 11:11, Joe Perches 写道:
> On Tue, 2013-01-08 at 10:57 +0800, Chen Gang wrote:
>>   sorry, I am not quite clear about what you said.
> 
> You found some overrun errors and proposed a patch.
> Your solution could output incomplete SSIDs.
> 
> I proposed a different patch that would fully output
> any binary/non-ascii printable SSID.
> 
> I also proposed a different mechanism to avoid the
> overrun via printk and also avoid possibly excessive
> stack consumption as a means for John Linville to
> select 1 of the above options.
> 
> I don't care what is done with any of those proposed
> patches.
> 
> Clear?
> 

  ok

-- 
Chen Gang

Asianux Corporation