diff mbox

[ARM] Use snprintf rather than sprintf where possible

Message ID 5640853C.2030306@arm.com
State New
Headers show

Commit Message

Kyrylo Tkachov Nov. 9, 2015, 11:36 a.m. UTC
Hi all,

Judging by the thread at https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01912.html
I looked at replacing calls to sprintf with calls to snprintf in the arm backend.
We use them a lot to print assembly mnemonics into static char buffers.
This patch replaces the calls with snprintf and adds a size argument equal to the size
of the buffer used. This way, if any of the format strings changes/increases past the size
of the allocated buffer, snprintf will truncate it (and the assembler will catch it) rather
than trying to write past the end of the buffer with unexpected results.

I managed to replace all uses of sprintf in the arm backend except the one in aout.h:
#define ASM_GENERATE_INTERNAL_LABEL(STRING, PREFIX, NUM)  \
   sprintf (STRING, "*%s%s%u", LOCAL_LABEL_PREFIX, PREFIX, (unsigned int)(NUM))

Here, ASM_GENERATE_INTERNAL_LABEL is used in various places in the midend to print labels
to static buffers. I've seen those buffers have sizes ranging from 12 chars to 256 chars.
The size of the buffer that ASM_GENERATE_INTERNAL_LABEL can depend on is not mandated in the
documentation or passed down to the macro, so I think this is a bit dangerous. In practice, however,
I don't think we print labels that long that that would cause an issue.

Bootstrapped and tested on arm-none-linux-gnueabihf.

Ok for trunk?

Thanks,
Kyrill

2015-11-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
     rather than sprintf.
     (arm_set_fixed_conv_libfunc): Likewise.
     (arm_option_override): Likewise.
     (neon_output_logic_immediate): Likewise.
     (neon_output_shift_immediate): Likewise.
     (arm_output_multireg_pop): Likewise.
     (vfp_output_vstmd): Likewise.
     (output_move_vfp): Likewise.
     (output_move_neon): Likewise.
     (output_return_instruction): Likewise.
     (arm_elf_asm_cdtor): Likewise.
     (arm_output_shift): Likewise.
     (arm_output_iwmmxt_shift_immediate): Likewise.
     (arm_output_iwmmxt_tinsr): Likewise.
     * config/arm/neon.md (*neon_mov<mode>, VDX): Likewise.
     (*neon_mov<mode>, VQXMOV): Likewise.
     (neon_vc<cmp_op><mode>_insn): Likewise.
     (neon_vc<cmp_op_unsp><mode>_insn_unspec): Likewise.

Comments

Kyrylo Tkachov Dec. 1, 2015, 4:14 p.m. UTC | #1
Ping.
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html

This fell through the cracks for me.
Is this ok at this stage? Or should I leave it for GCC 7?

Thanks,
Kyrill

On 09/11/15 11:36, Kyrill Tkachov wrote:
> Hi all,

>

> Judging by the thread at https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01912.html

> I looked at replacing calls to sprintf with calls to snprintf in the arm backend.

> We use them a lot to print assembly mnemonics into static char buffers.

> This patch replaces the calls with snprintf and adds a size argument equal to the size

> of the buffer used. This way, if any of the format strings changes/increases past the size

> of the allocated buffer, snprintf will truncate it (and the assembler will catch it) rather

> than trying to write past the end of the buffer with unexpected results.

>

> I managed to replace all uses of sprintf in the arm backend except the one in aout.h:

> #define ASM_GENERATE_INTERNAL_LABEL(STRING, PREFIX, NUM)  \

>   sprintf (STRING, "*%s%s%u", LOCAL_LABEL_PREFIX, PREFIX, (unsigned int)(NUM))

>

> Here, ASM_GENERATE_INTERNAL_LABEL is used in various places in the midend to print labels

> to static buffers. I've seen those buffers have sizes ranging from 12 chars to 256 chars.

> The size of the buffer that ASM_GENERATE_INTERNAL_LABEL can depend on is not mandated in the

> documentation or passed down to the macro, so I think this is a bit dangerous. In practice, however,

> I don't think we print labels that long that that would cause an issue.

>

> Bootstrapped and tested on arm-none-linux-gnueabihf.

>

> Ok for trunk?

>

> Thanks,

> Kyrill

>

> 2015-11-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

>

>     * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf

>     rather than sprintf.

>     (arm_set_fixed_conv_libfunc): Likewise.

>     (arm_option_override): Likewise.

>     (neon_output_logic_immediate): Likewise.

>     (neon_output_shift_immediate): Likewise.

>     (arm_output_multireg_pop): Likewise.

>     (vfp_output_vstmd): Likewise.

>     (output_move_vfp): Likewise.

>     (output_move_neon): Likewise.

>     (output_return_instruction): Likewise.

>     (arm_elf_asm_cdtor): Likewise.

>     (arm_output_shift): Likewise.

>     (arm_output_iwmmxt_shift_immediate): Likewise.

>     (arm_output_iwmmxt_tinsr): Likewise.

>     * config/arm/neon.md (*neon_mov<mode>, VDX): Likewise.

>     (*neon_mov<mode>, VQXMOV): Likewise.

>     (neon_vc<cmp_op><mode>_insn): Likewise.

>     (neon_vc<cmp_op_unsp><mode>_insn_unspec): Likewise.
diff mbox

Patch

commit bed313cb485dac52a5bb9984b55c9985af07ab35
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed Oct 21 14:40:07 2015 +0100

    [ARM] Use snprintf rather than sprintf

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index f86ebd9..fa34a7b 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -2303,9 +2303,10 @@  arm_set_fixed_optab_libfunc (optab optable, machine_mode mode,
   char buffer[50];
 
   if (num_suffix == 0)
-    sprintf (buffer, "__gnu_%s%s", funcname, modename);
+    snprintf (buffer, sizeof (buffer), "__gnu_%s%s", funcname, modename);
   else
-    sprintf (buffer, "__gnu_%s%s%d", funcname, modename, num_suffix);
+    snprintf (buffer, sizeof (buffer), "__gnu_%s%s%d", funcname,
+	      modename, num_suffix);
 
   set_optab_libfunc (optable, mode, buffer);
 }
@@ -2324,8 +2325,8 @@  arm_set_fixed_conv_libfunc (convert_optab optable, machine_mode to,
       && ALL_FRACT_MODE_P (from) == ALL_FRACT_MODE_P (to))
     maybe_suffix_2 = "2";
 
-  sprintf (buffer, "__gnu_%s%s%s%s", funcname, fromname, toname,
-	   maybe_suffix_2);
+  snprintf (buffer, sizeof (buffer), "__gnu_%s%s%s%s", funcname,
+	    fromname, toname, maybe_suffix_2);
 
   set_conv_libfunc (optable, to, from, buffer);
 }
@@ -3036,7 +3037,8 @@  arm_option_override (void)
   if (!arm_selected_tune)
     arm_selected_tune = &all_cores[arm_selected_cpu->core];
 
-  sprintf (arm_arch_name, "__ARM_ARCH_%s__", arm_selected_cpu->arch);
+  snprintf (arm_arch_name, sizeof (arm_arch_name),
+	    "__ARM_ARCH_%s__", arm_selected_cpu->arch);
   insn_flags = arm_selected_cpu->flags;
   arm_base_arch = arm_selected_cpu->base_arch;
 
@@ -12631,9 +12633,11 @@  neon_output_logic_immediate (const char *mnem, rtx *op2, machine_mode mode,
   gcc_assert (is_valid != 0);
 
   if (quad)
-    sprintf (templ, "%s.i%d\t%%q0, %%2", mnem, width);
+    snprintf (templ, sizeof (templ), "%s.i%d\t%%q0, %%2",
+	      mnem, width);
   else
-    sprintf (templ, "%s.i%d\t%%P0, %%2", mnem, width);
+    snprintf (templ, sizeof (templ), "%s.i%d\t%%P0, %%2",
+	      mnem, width);
 
   return templ;
 }
@@ -12653,9 +12657,11 @@  neon_output_shift_immediate (const char *mnem, char sign, rtx *op2,
   gcc_assert (is_valid != 0);
 
   if (quad)
-    sprintf (templ, "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
+    snprintf (templ, sizeof (templ),
+	      "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
   else
-    sprintf (templ, "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
+    snprintf (templ, sizeof (templ),
+	      "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
 
   return templ;
 }
@@ -17672,9 +17678,7 @@  arm_output_multireg_pop (rtx *operands, bool return_pc, rtx cond, bool reverse,
 
   conditional = reverse ? "%?%D0" : "%?%d0";
   if ((regno_base == SP_REGNUM) && update)
-    {
-      sprintf (pattern, "pop%s\t{", conditional);
-    }
+    snprintf (pattern, sizeof (pattern), "pop%s\t{", conditional);
   else
     {
       /* Output ldmfd when the base register is SP, otherwise output ldmia.
@@ -17682,12 +17686,12 @@  arm_output_multireg_pop (rtx *operands, bool return_pc, rtx cond, bool reverse,
       if (regno_base == SP_REGNUM)
 	  /* update is never true here, hence there is no need to handle
 	     pop here.  */
-	sprintf (pattern, "ldmfd%s", conditional);
+	snprintf (pattern, sizeof (pattern), "ldmfd%s", conditional);
 
       if (update)
-	sprintf (pattern, "ldmia%s\t", conditional);
+	snprintf (pattern, sizeof (pattern), "ldmia%s\t", conditional);
       else
-	sprintf (pattern, "ldm%s\t", conditional);
+	snprintf (pattern, sizeof (pattern), "ldm%s\t", conditional);
 
       strcat (pattern, reg_names[regno_base]);
       if (update)
@@ -17743,7 +17747,8 @@  vfp_output_vstmd (rtx * operands)
   base = (REGNO (operands[1]) - FIRST_VFP_REGNUM) / 2;
   for (i = 1; i < XVECLEN (operands[2], 0); i++)
     {
-      p += sprintf (&pattern[p], ", d%d", base + i);
+      p += snprintf (&pattern[p], sizeof (pattern),
+		     ", d%d", base + i);
     }
   strcpy (&pattern[p], "}");
 
@@ -18572,7 +18577,7 @@  output_move_vfp (rtx *operands)
       break;
     }
 
-  sprintf (buff, templ,
+  snprintf (buff, sizeof (buff), templ,
 	   load ? "ld" : "st",
 	   dp ? "64" : "32",
 	   dp ? "P" : "",
@@ -18712,7 +18717,8 @@  output_move_neon (rtx *operands)
 	      }
 	    else
 	      {
-		sprintf (buff, "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
+		snprintf (buff, sizeof (buff),
+			  "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
 		output_asm_insn (buff, ops);
 	      }
 	  }
@@ -18720,7 +18726,8 @@  output_move_neon (rtx *operands)
 	  {
 	    ops[0] = gen_rtx_REG (DImode, REGNO (reg) + 2 * overlap);
 	    ops[1] = adjust_address (mem, SImode, 8 * overlap);
-	    sprintf (buff, "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
+	    snprintf (buff, sizeof (buff),
+		      "v%sr%%?\t%%P0, %%1", load ? "ld" : "st");
 	    output_asm_insn (buff, ops);
 	  }
 
@@ -18731,7 +18738,7 @@  output_move_neon (rtx *operands)
       gcc_unreachable ();
     }
 
-  sprintf (buff, templ, load ? "ld" : "st");
+  snprintf (buff, sizeof (buff), templ, load ? "ld" : "st");
   output_asm_insn (buff, ops);
 
   return "";
@@ -19474,7 +19481,8 @@  output_return_instruction (rtx operand, bool really_return, bool reverse,
 
   gcc_assert (!cfun->calls_alloca || really_return);
 
-  sprintf (conditional, "%%?%%%c0", reverse ? 'D' : 'd');
+  snprintf (conditional, sizeof (conditional),
+	   "%%?%%%c0", reverse ? 'D' : 'd');
 
   cfun->machine->return_used_this_function = 1;
 
@@ -19525,7 +19533,8 @@  output_return_instruction (rtx operand, bool really_return, bool reverse,
 	      || ! really_return
 	      || ! IS_INTERRUPT (func_type)))
 	{
-	  sprintf (instr, "ldr%s\t%%|%s, [%%|sp], #4", conditional,
+	  snprintf (instr, sizeof (instr),
+		   "ldr%s\t%%|%s, [%%|sp], #4", conditional,
 		   (reg == LR_REGNUM) ? return_reg : reg_names[reg]);
 	}
       else
@@ -19545,7 +19554,8 @@  output_return_instruction (rtx operand, bool really_return, bool reverse,
 	      gcc_assert (stack_adjust == 0 || stack_adjust == 4);
 
 	      if (stack_adjust && arm_arch5 && TARGET_ARM)
-		  sprintf (instr, "ldmib%s\t%%|sp, {", conditional);
+		  snprintf (instr, sizeof (instr), "ldmib%s\t%%|sp, {",
+			    conditional);
 	      else
 		{
 		  /* If we can't use ldmib (SA110 bug),
@@ -19553,13 +19563,16 @@  output_return_instruction (rtx operand, bool really_return, bool reverse,
 		  if (stack_adjust)
 		    live_regs_mask |= 1 << 3;
 
-		  sprintf (instr, "ldmfd%s\t%%|sp, {", conditional);
+		  snprintf (instr, sizeof (instr), "ldmfdf%s\t%%|sp, {",
+			    conditional);
 		}
 	    }
 	  else
-	      sprintf (instr, "pop%s\t{", conditional);
+	      snprintf (instr, sizeof (instr), "pop%s\t{", conditional);
 
-	  p = instr + strlen (instr);
+	  size_t len = strlen (instr);
+	  p = instr + len;
+	  size_t rem_size = sizeof (instr) - len;
 
 	  for (reg = 0; reg <= SP_REGNUM; reg++)
 	    if (live_regs_mask & (1 << reg))
@@ -19581,7 +19594,7 @@  output_return_instruction (rtx operand, bool really_return, bool reverse,
 
 	  if (live_regs_mask & (1 << LR_REGNUM))
 	    {
-	      sprintf (p, "%s%%|%s}", first ? "" : ", ", return_reg);
+	      snprintf (p, rem_size, "%s%%|%s}", first ? "" : ", ", return_reg);
 	      /* If returning from an interrupt, restore the CPSR.  */
 	      if (IS_INTERRUPT (func_type))
 		strcat (p, "^");
@@ -19611,24 +19624,27 @@  output_return_instruction (rtx operand, bool really_return, bool reverse,
 	case ARM_FT_ISR:
 	case ARM_FT_FIQ:
 	  /* ??? This is wrong for unified assembly syntax.  */
-	  sprintf (instr, "sub%ss\t%%|pc, %%|lr, #4", conditional);
+	  snprintf (instr, sizeof (instr), "sub%ss\t%%|pc, %%|lr, #4",
+		    conditional);
 	  break;
 
 	case ARM_FT_INTERWORKED:
-	  sprintf (instr, "bx%s\t%%|lr", conditional);
+	  snprintf (instr, sizeof (instr), "bx%s\t%%|lr", conditional);
 	  break;
 
 	case ARM_FT_EXCEPTION:
 	  /* ??? This is wrong for unified assembly syntax.  */
-	  sprintf (instr, "mov%ss\t%%|pc, %%|lr", conditional);
+	  snprintf (instr, sizeof (instr), "mov%ss\t%%|pc, %%|lr",
+		    conditional);
 	  break;
 
 	default:
 	  /* Use bx if it's available.  */
 	  if (arm_arch5 || arm_arch4t)
-	    sprintf (instr, "bx%s\t%%|lr", conditional);
+	    snprintf (instr, sizeof (instr), "bx%s\t%%|lr", conditional);
 	  else
-	    sprintf (instr, "mov%s\t%%|pc, %%|lr", conditional);
+	    snprintf (instr, sizeof (instr), "mov%s\t%%|pc, %%|lr",
+		      conditional);
 	  break;
 	}
 
@@ -22679,9 +22695,9 @@  arm_elf_asm_cdtor (rtx symbol, int priority, bool is_ctor)
   if (priority != DEFAULT_INIT_PRIORITY)
     {
       char buf[18];
-      sprintf (buf, "%s.%.5u",
-	       is_ctor ? ".init_array" : ".fini_array",
-	       priority);
+      snprintf (buf, sizeof (buf), "%s.%.5u",
+		is_ctor ? ".init_array" : ".fini_array",
+		priority);
       s = get_section (buf, SECTION_WRITE, NULL_TREE);
     }
   else if (is_ctor)
@@ -27254,10 +27270,10 @@  arm_output_shift(rtx * operands, int set_flags)
     {
       if (val != -1)
 	operands[2] = GEN_INT(val);
-      sprintf (pattern, "%s%%%c\t%%0, %%1, %%2", shift, c);
+      snprintf (pattern, sizeof (pattern), "%s%%%c\t%%0, %%1, %%2", shift, c);
     }
   else
-    sprintf (pattern, "mov%%%c\t%%0, %%1", c);
+    snprintf (pattern, sizeof (pattern), "mov%%%c\t%%0, %%1", c);
 
   output_asm_insn (pattern, operands);
   return "";
@@ -27281,33 +27297,35 @@  arm_output_iwmmxt_shift_immediate (const char *insn_name, rtx *operands, bool wr
   {
     if (wror_or_wsra)
       {
-        sprintf (templ, "%s\t%%0, %%1, #%d", insn_name, 32);
-        output_asm_insn (templ, operands);
-        if (opmode == DImode)
-          {
-	    sprintf (templ, "%s\t%%0, %%0, #%d", insn_name, 32);
+	snprintf (templ, sizeof (templ), "%s\t%%0, %%1, #%d", insn_name, 32);
+	output_asm_insn (templ, operands);
+	if (opmode == DImode)
+	  {
+	    snprintf (templ, sizeof (templ), "%s\t%%0, %%0, #%d",
+		      insn_name, 32);
 	    output_asm_insn (templ, operands);
-          }
+	  }
       }
     else
       {
-        /* The destination register will contain all zeros.  */
-        sprintf (templ, "wzero\t%%0");
-        output_asm_insn (templ, operands);
+	/* The destination register will contain all zeros.  */
+	snprintf (templ, sizeof (templ), "wzero\t%%0");
+	output_asm_insn (templ, operands);
       }
     return "";
   }
 
   if ((opmode == DImode) && (shift > 32))
     {
-      sprintf (templ, "%s\t%%0, %%1, #%d", insn_name, 32);
+      snprintf (templ, sizeof (templ), "%s\t%%0, %%1, #%d", insn_name, 32);
       output_asm_insn (templ, operands);
-      sprintf (templ, "%s\t%%0, %%0, #%d", insn_name, shift - 32);
+      snprintf (templ, sizeof (templ), "%s\t%%0, %%0, #%d",
+		insn_name, shift - 32);
       output_asm_insn (templ, operands);
     }
   else
     {
-      sprintf (templ, "%s\t%%0, %%1, #%d", insn_name, shift);
+      snprintf (templ, sizeof (templ), "%s\t%%0, %%1, #%d", insn_name, shift);
       output_asm_insn (templ, operands);
     }
   return "";
@@ -27335,13 +27353,13 @@  arm_output_iwmmxt_tinsr (rtx *operands)
     switch (GET_MODE (operands[0]))
       {
       case V8QImode:
-	sprintf (templ, "tinsrb%%?\t%%0, %%2, #%d", i);
+	snprintf (templ, sizeof (templ), "tinsrb%%?\t%%0, %%2, #%d", i);
 	break;
       case V4HImode:
-	sprintf (templ, "tinsrh%%?\t%%0, %%2, #%d", i);
+	snprintf (templ, sizeof (templ), "tinsrh%%?\t%%0, %%2, #%d", i);
 	break;
       case V2SImode:
-	sprintf (templ, "tinsrw%%?\t%%0, %%2, #%d", i);
+	snprintf (templ, sizeof (templ), "tinsrw%%?\t%%0, %%2, #%d", i);
 	break;
       default:
 	gcc_unreachable ();
diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
index e5a2b0f..b674ec6 100644
--- a/gcc/config/arm/neon.md
+++ b/gcc/config/arm/neon.md
@@ -43,9 +43,10 @@  (define_insn "*neon_mov<mode>"
       gcc_assert (is_valid != 0);
 
       if (width == 0)
-        return "vmov.f32\t%P0, %1  @ <mode>";
+	return "vmov.f32\t%P0, %1  @ <mode>";
       else
-        sprintf (templ, "vmov.i%d\t%%P0, %%x1  @ <mode>", width);
+	snprintf (templ, sizeof (templ),
+		  "vmov.i%d\t%%P0, %%x1  @ <mode>", width);
 
       return templ;
     }
@@ -88,9 +89,10 @@  (define_insn "*neon_mov<mode>"
       gcc_assert (is_valid != 0);
 
       if (width == 0)
-        return "vmov.f32\t%q0, %1  @ <mode>";
+	return "vmov.f32\t%q0, %1  @ <mode>";
       else
-        sprintf (templ, "vmov.i%d\t%%q0, %%1  @ <mode>", width);
+	snprintf (templ, sizeof (templ),
+		  "vmov.i%d\t%%q0, %%1  @ <mode>", width);
 
       return templ;
     }
@@ -2187,12 +2189,13 @@  (define_insn "neon_vc<cmp_op><mode>_insn"
                     && !flag_unsafe_math_optimizations)"
   {
     char pattern[100];
-    sprintf (pattern, "vc<cmp_op>.%s%%#<V_sz_elem>\t%%<V_reg>0,"
-                      " %%<V_reg>1, %s",
-                       GET_MODE_CLASS (<MODE>mode) == MODE_VECTOR_FLOAT
-                         ? "f" : "<cmp_type>",
-                       which_alternative == 0
-                         ? "%<V_reg>2" : "#0");
+    snprintf (pattern, sizeof (pattern),
+	      "vc<cmp_op>.%s%%#<V_sz_elem>\t%%<V_reg>0,"
+	      " %%<V_reg>1, %s",
+	      GET_MODE_CLASS (<MODE>mode) == MODE_VECTOR_FLOAT
+		? "f" : "<cmp_type>",
+	      which_alternative == 0
+		? "%<V_reg>2" : "#0");
     output_asm_insn (pattern, operands);
     return "";
   }
@@ -2211,10 +2214,11 @@  (define_insn "neon_vc<cmp_op_unsp><mode>_insn_unspec"
   "TARGET_NEON"
   {
     char pattern[100];
-    sprintf (pattern, "vc<cmp_op_unsp>.f%%#<V_sz_elem>\t%%<V_reg>0,"
-                       " %%<V_reg>1, %s",
-                       which_alternative == 0
-                         ? "%<V_reg>2" : "#0");
+    snprintf (pattern, sizeof (pattern),
+	      "vc<cmp_op_unsp>.f%%#<V_sz_elem>\t%%<V_reg>0,"
+	      " %%<V_reg>1, %s",
+	      which_alternative == 0
+		? "%<V_reg>2" : "#0");
     output_asm_insn (pattern, operands);
     return "";
 }