Fix build warnings in locale/programs/ld-ctype.c

Message ID 45faf360-cad3-7c9b-a914-0823d2724b90@linux.ibm.com
State New
Headers show
Series
  • Fix build warnings in locale/programs/ld-ctype.c
Related show

Commit Message

Stefan Liebler June 25, 2019, 1:17 p.m.
Hi,

this patch fixes the gcc warnings seen with gcc 9 -march>=z13 on s390x:
programs/ld-ctype.c: In function ‘ctype_read’:
programs/ld-ctype.c:1392:13: error: ‘wch’ may be used uninitialized in 
this function [-Werror=maybe-uninitialized]
  1392 |    uint32_t wch;
       |             ^~~
programs/ld-ctype.c:1401:7: error: ‘seq’ may be used uninitialized in 
this function [-Werror=maybe-uninitialized]
  1401 |    if (seq != NULL && seq->nbytes == 1)
       |       ^
programs/ld-ctype.c:1391:20: note: ‘seq’ was declared here
  1391 |    struct charseq *seq;
       |                    ^~~

Both seq and wch are uninitialized if get_character fails.
Thus we are now returning with an error.

Bye
Stefan

ChangeLog:

	* locale/programs/ld-ctype.c (charclass_symbolic_ellipsis):
	Return error if get_character fails.

Comments

Florian Weimer June 25, 2019, 1:23 p.m. | #1
* Stefan Liebler:

> diff --git a/locale/programs/ld-ctype.c b/locale/programs/ld-ctype.c

> index e6105928da..cfc9c43fd5 100644

> --- a/locale/programs/ld-ctype.c

> +++ b/locale/programs/ld-ctype.c

> @@ -1396,7 +1396,8 @@ charclass_symbolic_ellipsis (struct linereader *ldfile,

>  		   (int) (now->val.str.lenmb - (cp - last_str)),

>  		   from);

>  

> -	  get_character (now, charmap, repertoire, &seq, &wch);

> +	  if (get_character (now, charmap, repertoire, &seq, &wch))

> +	    goto invalid_range;


Maybe write:

  if (get_character (now, charmap, repertoire, &seq, &wch) != 0)

to match the other function calls?

Otherwise, looks good.

Thanks,
Florian
Stefan Liebler June 25, 2019, 1:33 p.m. | #2
On 6/25/19 3:23 PM, Florian Weimer wrote:
> * Stefan Liebler:

> 

>> diff --git a/locale/programs/ld-ctype.c b/locale/programs/ld-ctype.c

>> index e6105928da..cfc9c43fd5 100644

>> --- a/locale/programs/ld-ctype.c

>> +++ b/locale/programs/ld-ctype.c

>> @@ -1396,7 +1396,8 @@ charclass_symbolic_ellipsis (struct linereader *ldfile,

>>   		   (int) (now->val.str.lenmb - (cp - last_str)),

>>   		   from);

>>   

>> -	  get_character (now, charmap, repertoire, &seq, &wch);

>> +	  if (get_character (now, charmap, repertoire, &seq, &wch))

>> +	    goto invalid_range;

> 

> Maybe write:

> 

>    if (get_character (now, charmap, repertoire, &seq, &wch) != 0)

> 

> to match the other function calls?

Okay. That's no problem. If no one opposes, I'll commit the patch 
tomorrow with "!= 0".

Shall I also update the following occurrence in ctype_read?
	      if (ellipsis_token == tok_none)
		{
		  if (get_character (now, charmap, repertoire, &seq, &wch))
		    goto err_label;

> 

> Otherwise, looks good.

> 

> Thanks,

> Florian

>
Florian Weimer June 25, 2019, 1:39 p.m. | #3
* Stefan Liebler:

> On 6/25/19 3:23 PM, Florian Weimer wrote:

>> * Stefan Liebler:

>>

>>> diff --git a/locale/programs/ld-ctype.c b/locale/programs/ld-ctype.c

>>> index e6105928da..cfc9c43fd5 100644

>>> --- a/locale/programs/ld-ctype.c

>>> +++ b/locale/programs/ld-ctype.c

>>> @@ -1396,7 +1396,8 @@ charclass_symbolic_ellipsis (struct linereader *ldfile,

>>>   		   (int) (now->val.str.lenmb - (cp - last_str)),

>>>   		   from);

>>>   -	  get_character (now, charmap, repertoire, &seq, &wch);

>>> +	  if (get_character (now, charmap, repertoire, &seq, &wch))

>>> +	    goto invalid_range;

>>

>> Maybe write:

>>

>>    if (get_character (now, charmap, repertoire, &seq, &wch) != 0)

>>

>> to match the other function calls?

> Okay. That's no problem. If no one opposes, I'll commit the patch

> tomorrow with "!= 0".

>

> Shall I also update the following occurrence in ctype_read?

> 	      if (ellipsis_token == tok_none)

> 		{

> 		  if (get_character (now, charmap, repertoire, &seq, &wch))

> 		    goto err_label;


Oh, I had missed that.  If the calls are already inconsistent, you can
use your original patch, too.

To be honest, I'm more concerned about the other calls to get_character
without error checking.

Thanks,
Florian
Stefan Liebler June 25, 2019, 1:49 p.m. | #4
On 6/25/19 3:39 PM, Florian Weimer wrote:
> * Stefan Liebler:

> 

>> On 6/25/19 3:23 PM, Florian Weimer wrote:

>>> * Stefan Liebler:

>>>

>>>> diff --git a/locale/programs/ld-ctype.c b/locale/programs/ld-ctype.c

>>>> index e6105928da..cfc9c43fd5 100644

>>>> --- a/locale/programs/ld-ctype.c

>>>> +++ b/locale/programs/ld-ctype.c

>>>> @@ -1396,7 +1396,8 @@ charclass_symbolic_ellipsis (struct linereader *ldfile,

>>>>    		   (int) (now->val.str.lenmb - (cp - last_str)),

>>>>    		   from);

>>>>    -	  get_character (now, charmap, repertoire, &seq, &wch);

>>>> +	  if (get_character (now, charmap, repertoire, &seq, &wch))

>>>> +	    goto invalid_range;

>>>

>>> Maybe write:

>>>

>>>     if (get_character (now, charmap, repertoire, &seq, &wch) != 0)

>>>

>>> to match the other function calls?

>> Okay. That's no problem. If no one opposes, I'll commit the patch

>> tomorrow with "!= 0".

>>

>> Shall I also update the following occurrence in ctype_read?

>> 	      if (ellipsis_token == tok_none)

>> 		{

>> 		  if (get_character (now, charmap, repertoire, &seq, &wch))

>> 		    goto err_label;

> 

> Oh, I had missed that.  If the calls are already inconsistent, you can

> use your original patch, too.

Okay. Then I'll use the original patch.
> 

> To be honest, I'm more concerned about the other calls to get_character

> without error checking.

Where do you see other ones?
With my patch applied, I just see the following occurrences of 
get_character which are now all checking the return value:
1257:get_character (struct token *now, const struct charmap_t *charmap,
1399:if (get_character (now, charmap, repertoire, &seq, &wch))
2291:if (get_character (now, charmap, repertoire, &seq, &wch))
2574:if (get_character (now, charmap, repertoire, &from_seq,
2585:if (get_character (now, charmap, repertoire, &to_seq,
> 

> Thanks,

> Florian

>
Florian Weimer June 25, 2019, 1:54 p.m. | #5
* Stefan Liebler:

>> To be honest, I'm more concerned about the other calls to get_character

>> without error checking.


> Where do you see other ones?

> With my patch applied, I just see the following occurrences of

> get_character which are now all checking the return value:

> 1257:get_character (struct token *now, const struct charmap_t *charmap,

> 1399:if (get_character (now, charmap, repertoire, &seq, &wch))

> 2291:if (get_character (now, charmap, repertoire, &seq, &wch))

> 2574:if (get_character (now, charmap, repertoire, &from_seq,

> 2585:if (get_character (now, charmap, repertoire, &to_seq,


Ah, my bad, you are right.  Please commit the original patch.

Thanks,
Florian
Stefan Liebler June 26, 2019, 10:32 a.m. | #6
On 6/25/19 3:54 PM, Florian Weimer wrote:
> * Stefan Liebler:

> 

>>> To be honest, I'm more concerned about the other calls to get_character

>>> without error checking.

> 

>> Where do you see other ones?

>> With my patch applied, I just see the following occurrences of

>> get_character which are now all checking the return value:

>> 1257:get_character (struct token *now, const struct charmap_t *charmap,

>> 1399:if (get_character (now, charmap, repertoire, &seq, &wch))

>> 2291:if (get_character (now, charmap, repertoire, &seq, &wch))

>> 2574:if (get_character (now, charmap, repertoire, &from_seq,

>> 2585:if (get_character (now, charmap, repertoire, &to_seq,

> 

> Ah, my bad, you are right.  Please commit the original patch.

> 

> Thanks,

> Florian

> 

Committed.

Thanks

Patch

commit 0dca38c4afd868da65e0b5c0d76fc30bd3114994
Author: Stefan Liebler <stli@linux.ibm.com>
Date:   Tue Jun 25 08:57:40 2019 +0200

    Fix build warnings in locale/programs/ld-ctype.c
    
    This patch fixes the gcc warnings seen with gcc 9 -march>=z13 on s390x:
    programs/ld-ctype.c: In function ‘ctype_read’:
    programs/ld-ctype.c:1392:13: error: ‘wch’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     1392 |    uint32_t wch;
          |             ^~~
    programs/ld-ctype.c:1401:7: error: ‘seq’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     1401 |    if (seq != NULL && seq->nbytes == 1)
          |       ^
    programs/ld-ctype.c:1391:20: note: ‘seq’ was declared here
     1391 |    struct charseq *seq;
          |                    ^~~
    
    Both seq and wch are uninitialized if get_character fails.
    Thus we are now returning with an error.
    
    ChangeLog:
    
            * locale/programs/ld-ctype.c (charclass_symbolic_ellipsis):
            Return error if get_character fails.

diff --git a/locale/programs/ld-ctype.c b/locale/programs/ld-ctype.c
index e6105928da..cfc9c43fd5 100644
--- a/locale/programs/ld-ctype.c
+++ b/locale/programs/ld-ctype.c
@@ -1396,7 +1396,8 @@  charclass_symbolic_ellipsis (struct linereader *ldfile,
 		   (int) (now->val.str.lenmb - (cp - last_str)),
 		   from);
 
-	  get_character (now, charmap, repertoire, &seq, &wch);
+	  if (get_character (now, charmap, repertoire, &seq, &wch))
+	    goto invalid_range;
 
 	  if (seq != NULL && seq->nbytes == 1)
 	    /* Yep, we can store information about this byte sequence.  */