diff mbox series

[v2,12/12] posix: Remove VLA usage for internal fnmatch implementation

Message ID 1517837254-19399-13-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series posix: glob/fnmatch fixes and refactor | expand

Commit Message

Adhemerval Zanella Feb. 5, 2018, 1:27 p.m. UTC 
Checked on x86_64-linux-gnu.

	* posix/fnmatch.c: Include char_array required files.
	* posix/fnmatch_loop.c (FCT): Replace VLA with char_array.

Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

---
 ChangeLog            |  3 +++
 posix/fnmatch.c      |  1 +
 posix/fnmatch_loop.c | 68 +++++++++++++++++++++++++---------------------------
 3 files changed, 36 insertions(+), 36 deletions(-)

-- 
2.7.4

Comments

Andreas Schwab Feb. 5, 2018, 1:40 p.m. UTC | #1 
On Feb 05 2018, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c

> index eadb343..831e5ba 100644

> --- a/posix/fnmatch_loop.c

> +++ b/posix/fnmatch_loop.c

> @@ -493,26 +493,20 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,

>  			  {

>  			    int32_t table_size;

>  			    const int32_t *symb_table;

> -# if WIDE_CHAR_VERSION

> -			    char str[c1];

> -			    unsigned int strcnt;

> -# else

> -#  define str (startp + 1)

> -# endif

> +			    struct char_array str;

> +			    char_array_init_empty (&str);

>  			    const unsigned char *extra;

>  			    int32_t idx;

>  			    int32_t elem;

>  			    int32_t second;

>  			    int32_t hash;

>  

> -# if WIDE_CHAR_VERSION

>  			    /* We have to convert the name to a single-byte

>  			       string.  This is possible since the names

>  			       consist of ASCII characters and the internal

>  			       representation is UCS4.  */

> -			    for (strcnt = 0; strcnt < c1; ++strcnt)

> -			      str[strcnt] = startp[1 + strcnt];

> -#endif

> +			    for (size_t strcnt = 0; strcnt < c1; ++strcnt)

> +			      char_array_append_char (&str, startp[1 + strcnt]);

>  

>  			    table_size =

>  			      _NL_CURRENT_WORD (LC_COLLATE,


That needs to be removed altogether, see
<http://sourceware.org/ml/libc-alpha/2017-04/msg00068.html>.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Adhemerval Zanella Feb. 5, 2018, 3:08 p.m. UTC | #2 
On 05/02/2018 11:40, Andreas Schwab wrote:
> On Feb 05 2018, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> 

>> diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c

>> index eadb343..831e5ba 100644

>> --- a/posix/fnmatch_loop.c

>> +++ b/posix/fnmatch_loop.c

>> @@ -493,26 +493,20 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,

>>  			  {

>>  			    int32_t table_size;

>>  			    const int32_t *symb_table;

>> -# if WIDE_CHAR_VERSION

>> -			    char str[c1];

>> -			    unsigned int strcnt;

>> -# else

>> -#  define str (startp + 1)

>> -# endif

>> +			    struct char_array str;

>> +			    char_array_init_empty (&str);

>>  			    const unsigned char *extra;

>>  			    int32_t idx;

>>  			    int32_t elem;

>>  			    int32_t second;

>>  			    int32_t hash;

>>  

>> -# if WIDE_CHAR_VERSION

>>  			    /* We have to convert the name to a single-byte

>>  			       string.  This is possible since the names

>>  			       consist of ASCII characters and the internal

>>  			       representation is UCS4.  */

>> -			    for (strcnt = 0; strcnt < c1; ++strcnt)

>> -			      str[strcnt] = startp[1 + strcnt];

>> -#endif

>> +			    for (size_t strcnt = 0; strcnt < c1; ++strcnt)

>> +			      char_array_append_char (&str, startp[1 + strcnt]);

>>  

>>  			    table_size =

>>  			      _NL_CURRENT_WORD (LC_COLLATE,

> 

> That needs to be removed altogether, see

> <http://sourceware.org/ml/libc-alpha/2017-04/msg00068.html>.


Nice, I missed your patch.  It seems to apply clean, I will review on the original
thread.
Adhemerval Zanella Feb. 5, 2018, 8:20 p.m. UTC | #3 
On 05/02/2018 13:08, Adhemerval Zanella wrote:
> 

> 

> On 05/02/2018 11:40, Andreas Schwab wrote:

>> On Feb 05 2018, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

>>

>>> diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c

>>> index eadb343..831e5ba 100644

>>> --- a/posix/fnmatch_loop.c

>>> +++ b/posix/fnmatch_loop.c

>>> @@ -493,26 +493,20 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,

>>>  			  {

>>>  			    int32_t table_size;

>>>  			    const int32_t *symb_table;

>>> -# if WIDE_CHAR_VERSION

>>> -			    char str[c1];

>>> -			    unsigned int strcnt;

>>> -# else

>>> -#  define str (startp + 1)

>>> -# endif

>>> +			    struct char_array str;

>>> +			    char_array_init_empty (&str);

>>>  			    const unsigned char *extra;

>>>  			    int32_t idx;

>>>  			    int32_t elem;

>>>  			    int32_t second;

>>>  			    int32_t hash;

>>>  

>>> -# if WIDE_CHAR_VERSION

>>>  			    /* We have to convert the name to a single-byte

>>>  			       string.  This is possible since the names

>>>  			       consist of ASCII characters and the internal

>>>  			       representation is UCS4.  */

>>> -			    for (strcnt = 0; strcnt < c1; ++strcnt)

>>> -			      str[strcnt] = startp[1 + strcnt];

>>> -#endif

>>> +			    for (size_t strcnt = 0; strcnt < c1; ++strcnt)

>>> +			      char_array_append_char (&str, startp[1 + strcnt]);

>>>  

>>>  			    table_size =

>>>  			      _NL_CURRENT_WORD (LC_COLLATE,

>>

>> That needs to be removed altogether, see

>> <http://sourceware.org/ml/libc-alpha/2017-04/msg00068.html>.

> 

> Nice, I missed your patch.  It seems to apply clean, I will review on the original

> thread.


Re-checking the bug reports related to the issues your patch aims to fix, I
noted on BZ#14185 comment #5 Jeff Law stated SuSE used to pack an out of tree
fix but non-longer do it. Do you know why exactly?
Andreas Schwab Feb. 6, 2018, 8:57 a.m. UTC | #4 
On Feb 05 2018, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> Re-checking the bug reports related to the issues your patch aims to fix, I

> noted on BZ#14185 comment #5 Jeff Law stated SuSE used to pack an out of tree

> fix but non-longer do it. Do you know why exactly?


I can't answer that question, I wasn't around at that time (the
changelog talks about "obsolete patch", which I don't quite follow).
But this bug is about matching the string, not processing the pattern,
so it is an unrelated issue.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
diff mbox series

Patch

diff --git a/posix/fnmatch.c b/posix/fnmatch.c
index 9c2cff0..b94c965 100644
--- a/posix/fnmatch.c
+++ b/posix/fnmatch.c
@@ -35,6 +35,7 @@ 
 #endif
 
 #include <scratch_buffer.h>
+#include <malloc/char_array-skeleton.c>
 
 /* For platform which support the ISO C amendement 1 functionality we
    support user defined character classes.  */
diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c
index eadb343..831e5ba 100644
--- a/posix/fnmatch_loop.c
+++ b/posix/fnmatch_loop.c
@@ -493,26 +493,20 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 			  {
 			    int32_t table_size;
 			    const int32_t *symb_table;
-# if WIDE_CHAR_VERSION
-			    char str[c1];
-			    unsigned int strcnt;
-# else
-#  define str (startp + 1)
-# endif
+			    struct char_array str;
+			    char_array_init_empty (&str);
 			    const unsigned char *extra;
 			    int32_t idx;
 			    int32_t elem;
 			    int32_t second;
 			    int32_t hash;
 
-# if WIDE_CHAR_VERSION
 			    /* We have to convert the name to a single-byte
 			       string.  This is possible since the names
 			       consist of ASCII characters and the internal
 			       representation is UCS4.  */
-			    for (strcnt = 0; strcnt < c1; ++strcnt)
-			      str[strcnt] = startp[1 + strcnt];
-#endif
+			    for (size_t strcnt = 0; strcnt < c1; ++strcnt)
+			      char_array_append_char (&str, startp[1 + strcnt]);
 
 			    table_size =
 			      _NL_CURRENT_WORD (LC_COLLATE,
@@ -525,7 +519,7 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 					   _NL_COLLATE_SYMB_EXTRAMB);
 
 			    /* Locate the character in the hashing table.  */
-			    hash = elem_hash (str, c1);
+			    hash = elem_hash (char_array_str (&str), c1);
 
 			    idx = 0;
 			    elem = hash % table_size;
@@ -539,7 +533,7 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 				    if (symb_table[2 * elem] == hash
 					&& (c1
 					    == extra[symb_table[2 * elem + 1]])
-					&& memcmp (str,
+					&& memcmp (char_array_str (&str),
 						   &extra[symb_table[2 * elem
 								     + 1]
 							  + 1], c1) == 0)
@@ -580,14 +574,20 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 					break;
 
 				    if ((int32_t) c1 == wextra[idx])
-				      goto matched;
+				      {
+					char_array_free (&str);
+				        goto matched;
+				      }
 # else
 				    for (c1 = 0; c1 < extra[idx]; ++c1)
 				      if (n[c1] != extra[1 + c1])
 					break;
 
 				    if (c1 == extra[idx])
-				      goto matched;
+				      {
+					char_array_free (&str);
+				        goto matched;
+				      }
 # endif
 				  }
 
@@ -608,18 +608,21 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 			      {
 				/* No valid character.  Match it as a
 				   single byte.  */
-				if (!is_range && *n == str[0])
-				  goto matched;
+				char str0 = *char_array_at (&str, 0);
+				if (!is_range && *n == str0)
+				  {
+				    char_array_free (&str);
+				    goto matched;
+				  }
 
-				cold = str[0];
+				cold = str0;
 				c = *p++;
 			      }
-			    else
-			      return FNM_NOMATCH;
+			    char_array_free (&str);
+			    return FNM_NOMATCH;
 			  }
 		      }
 		    else
-# undef str
 #endif
 		      {
 			c = FOLD (c);
@@ -711,26 +714,20 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 			      {
 				int32_t table_size;
 				const int32_t *symb_table;
-# if WIDE_CHAR_VERSION
-				char str[c1];
-				unsigned int strcnt;
-# else
-#  define str (startp + 1)
-# endif
+				struct char_array str;
+				char_array_init_empty (&str);
 				const unsigned char *extra;
 				int32_t idx;
 				int32_t elem;
 				int32_t second;
 				int32_t hash;
 
-# if WIDE_CHAR_VERSION
 				/* We have to convert the name to a single-byte
 				   string.  This is possible since the names
 				   consist of ASCII characters and the internal
 				   representation is UCS4.  */
-				for (strcnt = 0; strcnt < c1; ++strcnt)
-				  str[strcnt] = startp[1 + strcnt];
-# endif
+				for (size_t strcnt = 0; strcnt < c1; ++strcnt)
+				  char_array_append_char (&str, startp[1 + strcnt]);
 
 				table_size =
 				  _NL_CURRENT_WORD (LC_COLLATE,
@@ -744,7 +741,7 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 
 				/* Locate the character in the hashing
 				   table.  */
-				hash = elem_hash (str, c1);
+				hash = elem_hash (char_array_str (&str), c1);
 
 				idx = 0;
 				elem = hash % table_size;
@@ -758,7 +755,7 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 					if (symb_table[2 * elem] == hash
 					    && (c1
 						== extra[symb_table[2 * elem + 1]])
-					    && memcmp (str,
+					    && memcmp (char_array_str (&str),
 						       &extra[symb_table[2 * elem + 1]
 							      + 1], c1) == 0)
 					  {
@@ -800,13 +797,12 @@  FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
 				  }
 				else if (symb_table[2 * elem] != 0 && c1 == 1)
 				  {
-				    cend = str[0];
+				    cend = *char_array_at (&str, 0);
 				    c = *p++;
 				  }
-				else
-				  return FNM_NOMATCH;
+				char_array_free (&str);
+				return FNM_NOMATCH;
 			      }
-# undef str
 			  }
 			else
 			  {