diff mbox

Fix for PR68159 in Libiberty Demangler (6)

Message ID CA6D41B5-FF4D-4614-9716-B4FBEC2A7D52@gmail.com
State New
Headers show

Commit Message

Marcel Böhme May 6, 2016, 6:14 a.m. UTC 
Hi,

This patches fixes 
* the stack overflow reported in PR68159 in cplus_demangle_print_callback,
* a potential stack overflow in d_demangle_callback
* a potential stack overflow in is_ctor_or_dtor, and
* six potential buffer overflows (initialise less memory than needed due to integer overflow).

The stack overflow reported in PR68159 occurs due to assigning an array too much memory from the stack (447kb).
Similar stack overflows might occur in the remaining five dynamically allocated arrays in this and the other two functions.

Since the array size is controlled from the mangled string, we better safeguard from integer overflows and thus buffer overflows for these six arrays.

The patch allocates memory from the heap (xmalloc) instead of from the stack (dynamic arrays, alloca), checks for integer overflows, and frees the allocated memory before function return / abort.

Best regards,
- Marcel

Comments

Jakub Jelinek May 6, 2016, 7:09 a.m. UTC | #1 
On Fri, May 06, 2016 at 02:14:31PM +0800, Marcel Böhme wrote:
> * the stack overflow reported in PR68159 in cplus_demangle_print_callback,
> * a potential stack overflow in d_demangle_callback
> * a potential stack overflow in is_ctor_or_dtor, and
> * six potential buffer overflows (initialise less memory than needed due to integer overflow).
> 
> The stack overflow reported in PR68159 occurs due to assigning an array too much memory from the stack (447kb).
> Similar stack overflows might occur in the remaining five dynamically allocated arrays in this and the other two functions.
> 
> Since the array size is controlled from the mangled string, we better safeguard from integer overflows and thus buffer overflows for these six arrays.
> 
> The patch allocates memory from the heap (xmalloc) instead of from the stack (dynamic arrays, alloca), checks for integer overflows, and frees the allocated memory before function return / abort.

I think this is very problematic, but I'll let others comment on in detail.

Have you tested the patch at all?

This file is used not just in the various tools like binutils or gdb, but
also in libstdc++, where it used e.g. in the std::terminate handler,
which I think can't just xmalloc_failed, that function can be called already
in out of memory situation, where heap allocation is not possible.

Furthermore, if I apply your patch and rebuild libstdc++, it stops working
altogether:
ldd -d -r ./libstdc++.so.6.0.22 
	linux-vdso.so.1 (0x00007ffe6053c000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f9de23fb000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f9de203a000)
	/lib64/ld-linux-x86-64.so.2 (0x00005585ecc1d000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f9de1e22000)
undefined symbol: xmalloc	(./libstdc++.so.6.0.22)
undefined symbol: xmalloc_failed	(./libstdc++.so.6.0.22)
(which is why I'm asking about what testing you've done).

> @@ -4125,26 +4111,26 @@ cplus_demangle_print_callback (int options,
>    struct d_print_info dpi;
>  
>    d_print_init (&dpi, callback, opaque, dc);
> +  
> +  if (dpi.num_copy_templates > INT_MAX / (int) sizeof (*dpi.copy_templates))
> +    xmalloc_failed(INT_MAX);
> +  dpi.copy_templates = 
> +      (struct d_print_template *) xmalloc (dpi.num_copy_templates 
> +			                  * sizeof (*dpi.copy_templates));

Ignoring the fact that this patch breaks libstdc++, there are also
formatting and other issues.  Missing space in between xmalloc_failed
and (, = should not appear at the end of line, but on the start of
next line and it should be indented by 2, not 4.  Why INT_MAX?
I'd have thought that the allocation size is computed in size_t and
thus it should be SIZE_MAX, (~(size_t) 0) or similar?

	Jakub
diff mbox

Patch

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 235941)
+++ ChangeLog	(working copy)
@@ -1,3 +1,14 @@ 
+2016-05-06  Marcel Böhme  <boehme.marcel@gmail.com>
+
+	PR c++/68159
+	* cp-demangle.c: Check for overflow and allocate arrays of user-defined 
+	size on the heap, not on the stack.
+	(CP_DYNAMIC_ARRAYS): Remove redundant definition.
+	(cplus_demangle_print_callback): Check for overflow. Allocate memory 
+	for two arrays on the heap. Free memory before return / exit.
+	(d_demangle_callback): Likewise.
+	(is_ctor_or_dtor): Likewise. 
+
 2016-05-02  Marcel Böhme  <boehme.marcel@gmail.com>
 
 	PR c++/70498
Index: cp-demangle.c
===================================================================
--- cp-demangle.c	(revision 235941)
+++ cp-demangle.c	(working copy)
@@ -186,20 +186,6 @@  static void d_init_info (const char *, int, size_t
 #define CP_STATIC_IF_GLIBCPP_V3
 #endif /* ! defined(IN_GLIBCPP_V3) */
 
-/* See if the compiler supports dynamic arrays.  */
-
-#ifdef __GNUC__
-#define CP_DYNAMIC_ARRAYS
-#else
-#ifdef __STDC__
-#ifdef __STDC_VERSION__
-#if __STDC_VERSION__ >= 199901L
-#define CP_DYNAMIC_ARRAYS
-#endif /* __STDC__VERSION >= 199901L */
-#endif /* defined (__STDC_VERSION__) */
-#endif /* defined (__STDC__) */
-#endif /* ! defined (__GNUC__) */
-
 /* We avoid pulling in the ctype tables, to prevent pulling in
    additional unresolved symbols when this code is used in a library.
    FIXME: Is this really a valid reason?  This comes from the original
@@ -4125,26 +4111,26 @@  cplus_demangle_print_callback (int options,
   struct d_print_info dpi;
 
   d_print_init (&dpi, callback, opaque, dc);
+  
+  if (dpi.num_copy_templates > INT_MAX / (int) sizeof (*dpi.copy_templates))
+    xmalloc_failed(INT_MAX);
+  dpi.copy_templates = 
+      (struct d_print_template *) xmalloc (dpi.num_copy_templates 
+			                  * sizeof (*dpi.copy_templates));
 
-  {
-#ifdef CP_DYNAMIC_ARRAYS
-    __extension__ struct d_saved_scope scopes[dpi.num_saved_scopes];
-    __extension__ struct d_print_template temps[dpi.num_copy_templates];
+  if (dpi.num_saved_scopes > INT_MAX / (int) sizeof (*dpi.saved_scopes))
+    xmalloc_failed(INT_MAX);
+  dpi.saved_scopes = 
+      (struct d_saved_scope *) xmalloc (dpi.num_saved_scopes 
+			                * sizeof (*dpi.saved_scopes));
 
-    dpi.saved_scopes = scopes;
-    dpi.copy_templates = temps;
-#else
-    dpi.saved_scopes = alloca (dpi.num_saved_scopes
-			       * sizeof (*dpi.saved_scopes));
-    dpi.copy_templates = alloca (dpi.num_copy_templates
-				 * sizeof (*dpi.copy_templates));
-#endif
+  d_print_comp (&dpi, options, dc);
 
-    d_print_comp (&dpi, options, dc);
-  }
-
   d_print_flush (&dpi);
 
+  free(dpi.copy_templates);
+  free(dpi.saved_scopes);
+
   return ! d_print_saw_error (&dpi);
 }
 
@@ -5945,18 +5931,17 @@  d_demangle_callback (const char *mangled, int opti
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), &di);
 
-  {
-#ifdef CP_DYNAMIC_ARRAYS
-    __extension__ struct demangle_component comps[di.num_comps];
-    __extension__ struct demangle_component *subs[di.num_subs];
+  if (di.num_comps > INT_MAX / (int) sizeof (*di.comps))
+    xmalloc_failed(INT_MAX);
+  di.comps = (struct demangle_component *) xmalloc (di.num_comps 
+						    * sizeof (*di.comps));
 
-    di.comps = comps;
-    di.subs = subs;
-#else
-    di.comps = alloca (di.num_comps * sizeof (*di.comps));
-    di.subs = alloca (di.num_subs * sizeof (*di.subs));
-#endif
+  if (di.num_subs > INT_MAX / (int) sizeof (*di.subs))
+    xmalloc_failed(INT_MAX);
+  di.subs = (struct demangle_component **) xmalloc (di.num_subs 
+						   * sizeof (*di.subs));
 
+  {
     switch (type)
       {
       case DCT_TYPE:
@@ -5977,6 +5962,8 @@  d_demangle_callback (const char *mangled, int opti
 	d_advance (&di, strlen (d_str (&di)));
 	break;
       default:
+	free (di.comps);
+	free (di.subs);
 	abort (); /* We have listed all the cases.  */
       }
 
@@ -5995,7 +5982,8 @@  d_demangle_callback (const char *mangled, int opti
              ? cplus_demangle_print_callback (options, dc, callback, opaque)
              : 0;
   }
-
+  free (di.comps);
+  free (di.subs);
   return status;
 }
 
@@ -6226,18 +6214,17 @@  is_ctor_or_dtor (const char *mangled,
 
   cplus_demangle_init_info (mangled, DMGL_GNU_V3, strlen (mangled), &di);
 
+  if (di.num_comps > INT_MAX / (int) sizeof (*di.comps))
+    xmalloc_failed(INT_MAX);
+  di.comps = (struct demangle_component *) xmalloc (di.num_comps 
+						    * sizeof (*di.comps));
+
+  if (di.num_subs > INT_MAX / (int) sizeof (*di.subs))
+    xmalloc_failed(INT_MAX);
+  di.subs = (struct demangle_component **) xmalloc (di.num_subs 
+						   * sizeof (*di.subs));
   {
-#ifdef CP_DYNAMIC_ARRAYS
-    __extension__ struct demangle_component comps[di.num_comps];
-    __extension__ struct demangle_component *subs[di.num_subs];
 
-    di.comps = comps;
-    di.subs = subs;
-#else
-    di.comps = alloca (di.num_comps * sizeof (*di.comps));
-    di.subs = alloca (di.num_subs * sizeof (*di.subs));
-#endif
-
     dc = cplus_demangle_mangled_name (&di, 1);
 
     /* Note that because we did not pass DMGL_PARAMS, we don't expect
@@ -6279,7 +6266,8 @@  is_ctor_or_dtor (const char *mangled,
 	  }
       }
   }
-
+  free (di.comps);
+  free (di.subs);
   return ret;
 }
 
Index: testsuite/demangle-expected
===================================================================
--- testsuite/demangle-expected	(revision 235941)
+++ testsuite/demangle-expected	(working copy)
@@ -4441,3 +4441,8 @@  __vt_90000000000cafebabe
 
 _Z80800000000000000000000
 _Z80800000000000000000000
+#
+# Tests stack overflow
+
+_ZZN5Eigen9DenseBaseINS_5BlockINS_6MatrixIdLin1ELin1ELi0ELin1ELin1EEELin1ELin1ELb1EEEE10lazyAssignINS_12CwiseUnaryOpIZZN6netops4expr6sampleINS9_12nn6_combinerIN5boost3mpl9vector2_cIiLi0ELi2EEENS9_11rowwise_addINS9_6matmulINS9_12input_matrixINSE_IiLi0ELi0EEENSC_6fusion7vector3INSK_7vector4IN4nnet8mat_sizeIdLi1EEESP_NSO_IiLi0EEENSO_IdLi0EEEEENSK_7vector7ISP_SP_SP_SP_SP_SP_SP_EENSN_11config_dataIN3nn66detail8nn6_modeILb1EEEEEEEEENS9_13weight_matrixINSD_6v_itemIN4mpl_4int_ILi0EEENSD_9vector1_cIiLi2EEELi0EEENSK_7vector6INSK_7vector2ISP_NSN_8vec_sizeIdLi1EEEEES1F_S1F_S1F_S1F_S1F_EEEEEENS13_INS14_INS16_ILi1EEES19_Li0EEES1G_EEEENS9_16logistic_sigmoidINSG_INSH_INS1N_INSG_INSH_INSI_INSE_IiLi0ELi1EEES11_EENS13_INS14_IS17_NS18_IiLi0EEELi0EEES1G_EEEENS13_INS14_IS1J_S1Q_Li0EEES1G_EEEEEENS13_INS14_IS17_NS18_IiLi1EEELi0EEES1G_EEEENS13_INS14_IS1J_S1Y_Li0EEES1G_EEEEEEEEEclINSL_INSM_INS_12SparseMatrixIdLi1EiEENS2_IdLin1ELin1ELi1ELin1ELin1EEENS2_IiLin1ELi1ELi0ELin1ELi1EEENS2_IdLin1ELi1ELi0ELin1ELi1EEEEENST_IS2A_S2A_S2A_S2A_S2A_S2A_S2A_EESZ_EENS1B_INS1C_INS_3MapIS2B_Li1ENS_6StrideILi0ELi0EEEEENS2H_INS2_IdLi1ELin1ELi1ELi1ELin1EEELi1ES2J_EEEES2N_S2N_S2N_S2N_S2N_EEZZNS9_6concatIJNSI_INSE_IiLi0ELi3EEES11_EENS9_7dropoutILb0ES19_NS1N_INS2P_IJNSG_INSH_INSI_INSE_IiLi1ELi0EEES11_EENS13_INS14_IS17_NS18_IiLi3EEELi0EEES1G_EEEENS13_INS14_IS1J_S2V_Li0EEES1G_EEEENSG_INSH_INSI_INSE_IiLi1ELi1EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi2EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi3EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi4EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi5EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi6EEES11_EES2X_EES30_EEEEEEEEES27_EEclIS2G_S2O_ZNS9_6detail11binary_contIS2G_S2O_S3T_NS13_INS14_IS17_NS18_IiLi4EEELi0EEES1G_EEZNSH_IS3T_S3Z_EclIS2G_S2O_ZNS3W_IS2G_S2O_S40_NS13_INS14_IS1J_S3X_Li0EEES1G_EEZNSG_IS40_S43_EclIS2G_S2O_ZNS1N_IS44_EclIS2G_S2O_ZNS9_7concat2IS2R_S46_EclIS2G_S2O_ZNS3W_IS2G_S2O_S49_NS13_INS14_IS17_NS18_IiLi5EEELi0EEES1G_EEZNSH_IS49_S4D_EclIS2G_S2O_ZNS3W_IS2G_S2O_S4E_NS13_INS14_IS1J_S4B_Li0EEES1G_EEZNSG_IS4E_S4H_EclIS2G_S2O_ZNS9_11concat_zeroIS4I_EclIS2G_S2O_ZNS9_20softmax_crossentropyIS4L_EclIS2G_S2O_ZNKS8_11derived_ptrIS4O_E5fpropIS2G_S2O_EEDaRKT_RKT0_EUlOS4T_OS4W_OT1_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_OT2_E_EEDaRKNS4Q_IS51_EERKNS4Q_IS56_EES4V_S4Y_OKT3_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_S57_E_EEDaS5B_S5E_S4V_S4Y_S5H_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E0_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_S57_E_EEDaS5B_S5E_S4V_S4Y_S5H_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_S57_E_EEDaS5B_S5E_S4V_S4Y_S5H_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_ENKUlmRS4T_E0_clINS4Q_IS27_EEEEDamS5R_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_ENKUlS4Z_S50_S52_E_clIRKS2G_RKS2O_KNS_22SparseTimeDenseProductIS2A_S2B_EEEEDaS4Z_S50_S52_EUldE_S64_EEEERS4_RKNS0_IS4T_EEE19__PRETTY_FUNCTION__
+_ZZN5Eigen9DenseBaseINS_5BlockINS_6MatrixIdLin1ELin1ELi0ELin1ELin1EEELin1ELin1ELb1EEEE10lazyAssignINS_12CwiseUnaryOpIZZN6netops4expr6sampleINS9_12nn6_combinerIN5boost3mpl9vector2_cIiLi0ELi2EEENS9_11rowwise_addINS9_6matmulINS9_12input_matrixINSE_IiLi0ELi0EEENSC_6fusion7vector3INSK_7vector4IN4nnet8mat_sizeIdLi1EEESP_NSO_IiLi0EEENSO_IdLi0EEEEENSK_7vector7ISP_SP_SP_SP_SP_SP_SP_EENSN_11config_dataIN3nn66detail8nn6_modeILb1EEEEEEEEENS9_13weight_matrixINSD_6v_itemIN4mpl_4int_ILi0EEENSD_9vector1_cIiLi2EEELi0EEENSK_7vector6INSK_7vector2ISP_NSN_8vec_sizeIdLi1EEEEES1F_S1F_S1F_S1F_S1F_EEEEEENS13_INS14_INS16_ILi1EEES19_Li0EEES1G_EEEENS9_16logistic_sigmoidINSG_INSH_INS1N_INSG_INSH_INSI_INSE_IiLi0ELi1EEES11_EENS13_INS14_IS17_NS18_IiLi0EEELi0EEES1G_EEEENS13_INS14_IS1J_S1Q_Li0EEES1G_EEEEEENS13_INS14_IS17_NS18_IiLi1EEELi0EEES1G_EEEENS13_INS14_IS1J_S1Y_Li0EEES1G_EEEEEEEEEclINSL_INSM_INS_12SparseMatrixIdLi1EiEENS2_IdLin1ELin1ELi1ELin1ELin1EEENS2_IiLin1ELi1ELi0ELin1ELi1EEENS2_IdLin1ELi1ELi0ELin1ELi1EEEEENST_IS2A_S2A_S2A_S2A_S2A_S2A_S2A_EESZ_EENS1B_INS1C_INS_3MapIS2B_Li1ENS_6StrideILi0ELi0EEEEENS2H_INS2_IdLi1ELin1ELi1ELi1ELin1EEELi1ES2J_EEEES2N_S2N_S2N_S2N_S2N_EEZZNS9_6concatIJNSI_INSE_IiLi0ELi3EEES11_EENS9_7dropoutILb0ES19_NS1N_INS2P_IJNSG_INSH_INSI_INSE_IiLi1ELi0EEES11_EENS13_INS14_IS17_NS18_IiLi3EEELi0EEES1G_EEEENS13_INS14_IS1J_S2V_Li0EEES1G_EEEENSG_INSH_INSI_INSE_IiLi1ELi1EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi2EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi3EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi4EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi5EEES11_EES2X_EES30_EENSG_INSH_INSI_INSE_IiLi1ELi6EEES11_EES2X_EES30_EEEEEEEEES27_EEclIS2G_S2O_ZNS9_6detail11binary_contIS2G_S2O_S3T_NS13_INS14_IS17_NS18_IiLi4EEELi0EEES1G_EEZNSH_IS3T_S3Z_EclIS2G_S2O_ZNS3W_IS2G_S2O_S40_NS13_INS14_IS1J_S3X_Li0EEES1G_EEZNSG_IS40_S43_EclIS2G_S2O_ZNS1N_IS44_EclIS2G_S2O_ZNS9_7concat2IS2R_S46_EclIS2G_S2O_ZNS3W_IS2G_S2O_S49_NS13_INS14_IS17_NS18_IiLi5EEELi0EEES1G_EEZNSH_IS49_S4D_EclIS2G_S2O_ZNS3W_IS2G_S2O_S4E_NS13_INS14_IS1J_S4B_Li0EEES1G_EEZNSG_IS4E_S4H_EclIS2G_S2O_ZNS9_11concat_zeroIS4I_EclIS2G_S2O_ZNS9_20softmax_crossentropyIS4L_EclIS2G_S2O_ZNKS8_11derived_ptrIS4O_E5fpropIS2G_S2O_EEDaRKT_RKT0_EUlOS4T_OS4W_OT1_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_OT2_E_EEDaRKNS4Q_IS51_EERKNS4Q_IS56_EES4V_S4Y_OKT3_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_S57_E_EEDaS5B_S5E_S4V_S4Y_S5H_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E0_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_S57_E_EEDaS5B_S5E_S4V_S4Y_S5H_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_EUlS4Z_S50_S52_S57_E_EEDaS5B_S5E_S4V_S4Y_S5H_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_ENKUlmRS4T_E0_clINS4Q_IS27_EEEEDamS5R_EUlS4Z_S50_S52_E_EEDaS4V_S4Y_S52_ENKUlS4Z_S50_S52_E_clIRKS2G_RKS2O_KNS_22SparseTimeDenseProductIS2A_S2B_EEEEDaS4Z_S50_S52_EUldE_S64_EEEERS4_RKNS0_IS4T_EEE19__PRETTY_FUNCTION__