diff mbox series

[v2] disas/cris: Pass buffer size to format_dec() to avoid overflow warning

Message ID 20231120214520.59431-1-pbonzini@redhat.com
State Superseded
Headers show
Series [v2] disas/cris: Pass buffer size to format_dec() to avoid overflow warning | expand

Commit Message

Paolo Bonzini Nov. 20, 2023, 9:45 p.m. UTC
From: Philippe Mathieu-Daudé <philmd@linaro.org>

Propagate the buffer size to format_dec() and use snprintf().

This should silence this UBSan -Wformat-overflow warning:

  In file included from /usr/include/stdio.h:906,
                   from include/qemu/osdep.h:114,
                   from ../disas/cris.c:21:
  In function 'sprintf',
      inlined from 'format_dec' at ../disas/cris.c:1737:3,
      inlined from 'print_with_operands' at ../disas/cris.c:2477:12,
      inlined from 'print_insn_cris_generic.constprop' at ../disas/cris.c:2690:8:
  /usr/include/bits/stdio2.h:30:10: warning: null destination pointer [-Wformat-overflow=]
   30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |                                   __glibc_objsize (__s), __fmt,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |                                   __va_arg_pack ());
      |                                   ~~~~~~~~~~~~~~~~~

Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20231120132222.82138-1-philmd@linaro.org>
[Rewritten to fix logic and avoid repeated expression. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 disas/cris.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Akihiko Odaki Nov. 21, 2023, 5:32 a.m. UTC | #1
On 2023/11/21 6:45, Paolo Bonzini wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Propagate the buffer size to format_dec() and use snprintf().
> 
> This should silence this UBSan -Wformat-overflow warning:
> 
>    In file included from /usr/include/stdio.h:906,
>                     from include/qemu/osdep.h:114,
>                     from ../disas/cris.c:21:
>    In function 'sprintf',
>        inlined from 'format_dec' at ../disas/cris.c:1737:3,
>        inlined from 'print_with_operands' at ../disas/cris.c:2477:12,
>        inlined from 'print_insn_cris_generic.constprop' at ../disas/cris.c:2690:8:
>    /usr/include/bits/stdio2.h:30:10: warning: null destination pointer [-Wformat-overflow=]
>     30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
>        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     31 |                                   __glibc_objsize (__s), __fmt,
>        |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     32 |                                   __va_arg_pack ());
>        |                                   ~~~~~~~~~~~~~~~~~
> 
> Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <20231120132222.82138-1-philmd@linaro.org>
> [Rewritten to fix logic and avoid repeated expression. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   disas/cris.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/disas/cris.c b/disas/cris.c
> index 0b0a3fb9165..409a224c5d1 100644
> --- a/disas/cris.c
> +++ b/disas/cris.c
> @@ -1731,10 +1731,10 @@ format_hex (unsigned long number,
>      unsigned (== 0).  */
>   
>   static char *
> -format_dec (long number, char *outbuffer, int signedp)
> +format_dec (long number, char *outbuffer, size_t outsize, int signedp)
>   {
>     last_immediate = number;
> -  sprintf (outbuffer, signedp ? "%ld" : "%lu", number);
> +  snprintf (outbuffer, outsize, signedp ? "%ld" : "%lu", number);
>   
>     return outbuffer + strlen (outbuffer);
>   }
> @@ -1876,6 +1876,12 @@ print_flags (struct cris_disasm_data *disdata, unsigned int insn, char *cp)
>     return cp;
>   }
>   
> +#define FORMAT_DEC(number, tp, signedp)                      \
> +    format_dec (number, tp, ({                                \

Nit: the slash at the end is misaligned.

Otherwise,
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

> +            assert(tp >= temp && tp <= temp + sizeof(temp)); \
> +            temp + sizeof(temp) - tp;                        \
> +        }), signedp)
> +
>   /* Print out an insn with its operands, and update the info->insn_type
>      fields.  The prefix_opcodep and the rest hold a prefix insn that is
>      supposed to be output as an address mode.  */
> @@ -2105,7 +2111,7 @@ print_with_operands (const struct cris_opcode *opcodep,
>   	    if ((*cs == 'z' && (insn & 0x20))
>   		|| (opcodep->match == BDAP_QUICK_OPCODE
>   		    && (nbytes <= 2 || buffer[1 + nbytes] == 0)))
> -	      tp = format_dec (number, tp, signedp);
> +	      tp = FORMAT_DEC (number, tp, signedp);
>   	    else
>   	      {
>   		unsigned int highbyte = (number >> 24) & 0xff;
> @@ -2241,7 +2247,7 @@ print_with_operands (const struct cris_opcode *opcodep,
>   				       with_reg_prefix);
>   		      if (number >= 0)
>   			*tp++ = '+';
> -		      tp = format_dec (number, tp, 1);
> +		      tp = FORMAT_DEC (number, tp, 1);
>   
>   		      info->flags |= CRIS_DIS_FLAG_MEM_TARGET_IS_REG;
>   		      info->target = (prefix_insn >> 12) & 15;
> @@ -2340,7 +2346,7 @@ print_with_operands (const struct cris_opcode *opcodep,
>   			  {
>   			    if (number >= 0)
>   			      *tp++ = '+';
> -			    tp = format_dec (number, tp, 1);
> +			    tp = FORMAT_DEC (number, tp, 1);
>   			  }
>   		      }
>   		    else
> @@ -2397,7 +2403,7 @@ print_with_operands (const struct cris_opcode *opcodep,
>   	break;
>   
>         case 'I':
> -	tp = format_dec (insn & 63, tp, 0);
> +	tp = FORMAT_DEC (insn & 63, tp, 0);
>   	break;
>   
>         case 'b':
> @@ -2426,11 +2432,11 @@ print_with_operands (const struct cris_opcode *opcodep,
>         break;
>   
>       case 'c':
> -      tp = format_dec (insn & 31, tp, 0);
> +      tp = FORMAT_DEC (insn & 31, tp, 0);
>         break;
>   
>       case 'C':
> -      tp = format_dec (insn & 15, tp, 0);
> +      tp = FORMAT_DEC (insn & 15, tp, 0);
>         break;
>   
>       case 'o':
> @@ -2463,7 +2469,7 @@ print_with_operands (const struct cris_opcode *opcodep,
>   	if (number > 127)
>   	  number = number - 256;
>   
> -	tp = format_dec (number, tp, 1);
> +	tp = FORMAT_DEC (number, tp, 1);
>   	*tp++ = ',';
>   	tp = format_reg (disdata, (insn >> 12) & 15, tp, with_reg_prefix);
>         }
> @@ -2474,7 +2480,7 @@ print_with_operands (const struct cris_opcode *opcodep,
>         break;
>   
>       case 'i':
> -      tp = format_dec ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1);
> +      tp = FORMAT_DEC ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1);
>         break;
>   
>       case 'P':
Philippe Mathieu-Daudé Nov. 21, 2023, 7:54 a.m. UTC | #2
On 20/11/23 22:45, Paolo Bonzini wrote:
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Propagate the buffer size to format_dec() and use snprintf().
> 
> This should silence this UBSan -Wformat-overflow warning:

"produced when using GCC 12.1.0:"

> 
>    In file included from /usr/include/stdio.h:906,
>                     from include/qemu/osdep.h:114,
>                     from ../disas/cris.c:21:
>    In function 'sprintf',
>        inlined from 'format_dec' at ../disas/cris.c:1737:3,
>        inlined from 'print_with_operands' at ../disas/cris.c:2477:12,
>        inlined from 'print_insn_cris_generic.constprop' at ../disas/cris.c:2690:8:
>    /usr/include/bits/stdio2.h:30:10: warning: null destination pointer [-Wformat-overflow=]
>     30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
>        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     31 |                                   __glibc_objsize (__s), __fmt,
>        |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     32 |                                   __va_arg_pack ());
>        |                                   ~~~~~~~~~~~~~~~~~
> 
> Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <20231120132222.82138-1-philmd@linaro.org>
> [Rewritten to fix logic and avoid repeated expression. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   disas/cris.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/disas/cris.c b/disas/cris.c
> index 0b0a3fb9165..409a224c5d1 100644
> --- a/disas/cris.c
> +++ b/disas/cris.c
> @@ -1731,10 +1731,10 @@ format_hex (unsigned long number,
>      unsigned (== 0).  */
>   
>   static char *
> -format_dec (long number, char *outbuffer, int signedp)
> +format_dec (long number, char *outbuffer, size_t outsize, int signedp)
>   {
>     last_immediate = number;
> -  sprintf (outbuffer, signedp ? "%ld" : "%lu", number);
> +  snprintf (outbuffer, outsize, signedp ? "%ld" : "%lu", number);
>   
>     return outbuffer + strlen (outbuffer);
>   }
> @@ -1876,6 +1876,12 @@ print_flags (struct cris_disasm_data *disdata, unsigned int insn, char *cp)
>     return cp;
>   }
>   
> +#define FORMAT_DEC(number, tp, signedp)                      \
> +    format_dec (number, tp, ({                                \
> +            assert(tp >= temp && tp <= temp + sizeof(temp)); \
> +            temp + sizeof(temp) - tp;                        \
> +        }), signedp)

Thank you Paolo for this respin!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff mbox series

Patch

diff --git a/disas/cris.c b/disas/cris.c
index 0b0a3fb9165..409a224c5d1 100644
--- a/disas/cris.c
+++ b/disas/cris.c
@@ -1731,10 +1731,10 @@  format_hex (unsigned long number,
    unsigned (== 0).  */
 
 static char *
-format_dec (long number, char *outbuffer, int signedp)
+format_dec (long number, char *outbuffer, size_t outsize, int signedp)
 {
   last_immediate = number;
-  sprintf (outbuffer, signedp ? "%ld" : "%lu", number);
+  snprintf (outbuffer, outsize, signedp ? "%ld" : "%lu", number);
 
   return outbuffer + strlen (outbuffer);
 }
@@ -1876,6 +1876,12 @@  print_flags (struct cris_disasm_data *disdata, unsigned int insn, char *cp)
   return cp;
 }
 
+#define FORMAT_DEC(number, tp, signedp)                      \
+    format_dec (number, tp, ({                                \
+            assert(tp >= temp && tp <= temp + sizeof(temp)); \
+            temp + sizeof(temp) - tp;                        \
+        }), signedp)
+
 /* Print out an insn with its operands, and update the info->insn_type
    fields.  The prefix_opcodep and the rest hold a prefix insn that is
    supposed to be output as an address mode.  */
@@ -2105,7 +2111,7 @@  print_with_operands (const struct cris_opcode *opcodep,
 	    if ((*cs == 'z' && (insn & 0x20))
 		|| (opcodep->match == BDAP_QUICK_OPCODE
 		    && (nbytes <= 2 || buffer[1 + nbytes] == 0)))
-	      tp = format_dec (number, tp, signedp);
+	      tp = FORMAT_DEC (number, tp, signedp);
 	    else
 	      {
 		unsigned int highbyte = (number >> 24) & 0xff;
@@ -2241,7 +2247,7 @@  print_with_operands (const struct cris_opcode *opcodep,
 				       with_reg_prefix);
 		      if (number >= 0)
 			*tp++ = '+';
-		      tp = format_dec (number, tp, 1);
+		      tp = FORMAT_DEC (number, tp, 1);
 
 		      info->flags |= CRIS_DIS_FLAG_MEM_TARGET_IS_REG;
 		      info->target = (prefix_insn >> 12) & 15;
@@ -2340,7 +2346,7 @@  print_with_operands (const struct cris_opcode *opcodep,
 			  {
 			    if (number >= 0)
 			      *tp++ = '+';
-			    tp = format_dec (number, tp, 1);
+			    tp = FORMAT_DEC (number, tp, 1);
 			  }
 		      }
 		    else
@@ -2397,7 +2403,7 @@  print_with_operands (const struct cris_opcode *opcodep,
 	break;
 
       case 'I':
-	tp = format_dec (insn & 63, tp, 0);
+	tp = FORMAT_DEC (insn & 63, tp, 0);
 	break;
 
       case 'b':
@@ -2426,11 +2432,11 @@  print_with_operands (const struct cris_opcode *opcodep,
       break;
 
     case 'c':
-      tp = format_dec (insn & 31, tp, 0);
+      tp = FORMAT_DEC (insn & 31, tp, 0);
       break;
 
     case 'C':
-      tp = format_dec (insn & 15, tp, 0);
+      tp = FORMAT_DEC (insn & 15, tp, 0);
       break;
 
     case 'o':
@@ -2463,7 +2469,7 @@  print_with_operands (const struct cris_opcode *opcodep,
 	if (number > 127)
 	  number = number - 256;
 
-	tp = format_dec (number, tp, 1);
+	tp = FORMAT_DEC (number, tp, 1);
 	*tp++ = ',';
 	tp = format_reg (disdata, (insn >> 12) & 15, tp, with_reg_prefix);
       }
@@ -2474,7 +2480,7 @@  print_with_operands (const struct cris_opcode *opcodep,
       break;
 
     case 'i':
-      tp = format_dec ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1);
+      tp = FORMAT_DEC ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1);
       break;
 
     case 'P':