Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-11-02 Thread Marek Polacek
On Fri, Nov 01, 2013 at 04:39:09PM -0400, Jason Merrill wrote: On 11/01/2013 03:10 PM, Marek Polacek wrote: + /* We need to stabilize side-effects in VLA sizes for regular array + declarations too, not just pointers to arrays. */ This comment isn't really relevant to its new

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-11-01 Thread Jason Merrill
On 10/31/2013 02:28 PM, Marek Polacek wrote: /* A variable sized array. */ itype = variable_size (itype); + + /* We need to stabilize side-effects in VLA sizes for regular array +declarations too, not just pointers to arrays. */ +

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-11-01 Thread Marek Polacek
On Fri, Nov 01, 2013 at 01:35:07PM -0400, Jason Merrill wrote: On 10/31/2013 02:28 PM, Marek Polacek wrote: /* A variable sized array. */ itype = variable_size (itype); + + /* We need to stabilize side-effects in VLA sizes for regular array + declarations too, not

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-11-01 Thread Jason Merrill
On 11/01/2013 03:10 PM, Marek Polacek wrote: + /* We need to stabilize side-effects in VLA sizes for regular array +declarations too, not just pointers to arrays. */ This comment isn't really relevant to its new location. :) OK with that removed. Jason

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-31 Thread Marek Polacek
On Wed, Oct 30, 2013 at 03:41:53PM -0700, Mike Stump wrote: The dtors only run, after the ctors run. We mark where the ctors finish spot, as the _start_ of the region for which we have to clean up. Really, the cleanup has nothing to do with ctors. You can have dtors, without any ctors,

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-31 Thread Marek Polacek
On Wed, Oct 30, 2013 at 09:10:30PM -0400, Jason Merrill wrote: On 10/30/2013 12:15 PM, Marek Polacek wrote: On Wed, Oct 30, 2013 at 11:56:25AM -0400, Jason Merrill wrote: Saving 'size' here doesn't help since it's already been used above. Could you use itype instead of size here? I already

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-30 Thread Marek Polacek
On Fri, Oct 25, 2013 at 03:04:41PM -0400, Jason Merrill wrote: I'm sorry, you want me to move the c++1y VLA check into compute_array_index_type, or just do the ubsan instrumentation in there? Thanks, Both. Unfortunately, I'm having quite a lot of trouble with side-effects. :( For e.g. int

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-30 Thread Jason Merrill
On 10/30/2013 10:52 AM, Marek Polacek wrote: + if ((flag_sanitize SANITIZE_VLA) + !processing_template_decl You don't need to check processing_template_decl; the template case was already handled above. + tree x = cp_save_expr (size); + x =

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-30 Thread Marek Polacek
On Wed, Oct 30, 2013 at 11:56:25AM -0400, Jason Merrill wrote: On 10/30/2013 10:52 AM, Marek Polacek wrote: + if ((flag_sanitize SANITIZE_VLA) + !processing_template_decl You don't need to check processing_template_decl; the template case was already handled above.

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-30 Thread Mike Stump
On Oct 30, 2013, at 9:15 AM, Marek Polacek pola...@redhat.com wrote: I admit I don't understand the cleanup_points very much and I don't know exactly where they are coming from So, here is the mental model… and how it is related to the standard. C++ mandates that destructors for objects and

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-30 Thread Marek Polacek
Thanks Mike. I had a quick look at the CLEANUP_STMT and cp-tree.def says A CLEANUP_STMT marks the point at which a declaration is fully constructed., while doc says Used to represent an action that should take place upon exit from the enclosing scope. Typically, these actions are calls to

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-30 Thread Mike Stump
On Oct 30, 2013, at 3:15 PM, Marek Polacek pola...@redhat.com wrote: I had a quick look at the CLEANUP_STMT and cp-tree.def says A CLEANUP_STMT marks the point at which a declaration is fully constructed., while doc says Used to represent an action that should take place upon exit from the

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-30 Thread Jason Merrill
On 10/30/2013 12:15 PM, Marek Polacek wrote: On Wed, Oct 30, 2013 at 11:56:25AM -0400, Jason Merrill wrote: Saving 'size' here doesn't help since it's already been used above. Could you use itype instead of size here? I already experimented with that and I think I can't, since we call the

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-25 Thread Marek Polacek
On Thu, Oct 24, 2013 at 03:57:17PM -0400, Jason Merrill wrote: On 09/25/2013 08:41 AM, Marek Polacek wrote: + /* Do the instrumentation of VLAs if desired. */ + if ((flag_sanitize SANITIZE_VLA) + size !TREE_CONSTANT (size) + /* From C++1y onwards, we throw an exception on a

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-25 Thread Jason Merrill
On 10/25/2013 12:58 PM, Marek Polacek wrote: I've tried to implement the instrumentation in cp_finish_decl. However, the problem is with multidimensional arrays, e.g. for int x = -1; int a[1][x]; array_of_runtime_bound_p returns false, thus we don't instrument this at all, nor throw an

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-25 Thread Marek Polacek
On Fri, Oct 25, 2013 at 02:17:48PM -0400, Jason Merrill wrote: On 10/25/2013 12:58 PM, Marek Polacek wrote: I've tried to implement the instrumentation in cp_finish_decl. However, the problem is with multidimensional arrays, e.g. for int x = -1; int a[1][x]; array_of_runtime_bound_p

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-25 Thread Jason Merrill
On 10/25/2013 03:03 PM, Marek Polacek wrote: On Fri, Oct 25, 2013 at 02:17:48PM -0400, Jason Merrill wrote: I think the right place to handle both ubsan and c++1y VLA checks is in compute_array_index_type, in the block where we're calling variable_size. I'm sorry, you want me to move the

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-24 Thread Jason Merrill
On 09/25/2013 08:41 AM, Marek Polacek wrote: + /* Do the instrumentation of VLAs if desired. */ + if ((flag_sanitize SANITIZE_VLA) + size !TREE_CONSTANT (size) + /* From C++1y onwards, we throw an exception on a negative length size + of an array. */ + cxx_dialect

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-15 Thread Marek Polacek
Ping^2. Jason, Joseph, are you fine with the C++/C FE changes? Thanks. On Mon, Oct 07, 2013 at 10:17:38PM +0200, Marek Polacek wrote: Ping. On Wed, Sep 25, 2013 at 02:41:32PM +0200, Marek Polacek wrote: On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote: This patch adds the

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-15 Thread Joseph S. Myers
On Tue, 15 Oct 2013, Marek Polacek wrote: Ping^2. Jason, Joseph, are you fine with the C++/C FE changes? The C changes are fine with me. -- Joseph S. Myers jos...@codesourcery.com

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-10-07 Thread Marek Polacek
Ping. On Wed, Sep 25, 2013 at 02:41:32PM +0200, Marek Polacek wrote: On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote: This patch adds the instrumentation of VLA bounds. Basically, it just checks that the size of a VLA is positive. I.e., We also issue an error if the size

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-25 Thread Marek Polacek
On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote: This patch adds the instrumentation of VLA bounds. Basically, it just checks that the size of a VLA is positive. I.e., We also issue an error if the size of the VLA is 0. It catches e.g. int i = 1; int a[i][i - 2]; It

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-16 Thread Marek Polacek
On Thu, Sep 12, 2013 at 04:05:48PM +, Joseph S. Myers wrote: On Thu, 12 Sep 2013, Joseph S. Myers wrote: (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not just SIZE_MAX, should be caught, because pointer subtraction cannot work reliably with larger objects.

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-16 Thread Florian Weimer
On 09/12/2013 06:05 PM, Joseph S. Myers wrote: On Thu, 12 Sep 2013, Joseph S. Myers wrote: (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not just SIZE_MAX, should be caught, because pointer subtraction cannot work reliably with larger objects. So it's not just when

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-13 Thread Marek Polacek
On Thu, Sep 12, 2013 at 04:05:48PM +, Joseph S. Myers wrote: cause stack overflow that doesn't get detected by the kernel. So maybe ubsan should imply -fstack-check or similar. Well, I have a patch for that, but I no longer think that ubsan should imply -fstack-check, since e.g. int

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-13 Thread Eric Botcazou
Well, I have a patch for that, but I no longer think that ubsan should imply -fstack-check, since e.g. int main (void) { int x = -1; int b[x - 4]; /* ... */ return 0; } segfaults at runtime on int b[x - 4]; line when -fstack-check is used (even without sanitizing), so we

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-13 Thread Joseph S. Myers
On Fri, 13 Sep 2013, Marek Polacek wrote: On Thu, Sep 12, 2013 at 04:05:48PM +, Joseph S. Myers wrote: cause stack overflow that doesn't get detected by the kernel. So maybe ubsan should imply -fstack-check or similar. Well, I have a patch for that, but I no longer think that ubsan

[PATCH][ubsan] Add VLA bound instrumentation

2013-09-12 Thread Marek Polacek
This patch adds the instrumentation of VLA bounds. Basically, it just checks that the size of a VLA is positive. I.e., We also issue an error if the size of the VLA is 0. It catches e.g. int i = 1; int a[i][i - 2]; It is pretty straightforward, but I had issues in the C++ FE, mainly choosing

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-12 Thread Marek Polacek
On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote: the size of a VLA is positive. I.e., We also issue an error if the size of the s/We also/we/

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-12 Thread Marek Polacek
On Thu, Sep 12, 2013 at 04:05:48PM +, Joseph S. Myers wrote: On Thu, 12 Sep 2013, Joseph S. Myers wrote: (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not just SIZE_MAX, should be caught, because pointer subtraction cannot work reliably with larger objects.

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-12 Thread Marek Polacek
On Thu, Sep 12, 2013 at 03:52:18PM +, Joseph S. Myers wrote: On Thu, 12 Sep 2013, Marek Polacek wrote: This patch adds the instrumentation of VLA bounds. Basically, it just checks that the size of a VLA is positive. I.e., We also issue an error if the size of the VLA is 0. It

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-12 Thread Joseph S. Myers
On Thu, 12 Sep 2013, Joseph S. Myers wrote: (Actually, I believe sizes (in bytes) greater than target PTRDIFF_MAX, not just SIZE_MAX, should be caught, because pointer subtraction cannot work reliably with larger objects. So it's not just when the size or multiplication overflow size_t,

Re: [PATCH][ubsan] Add VLA bound instrumentation

2013-09-12 Thread Joseph S. Myers
On Thu, 12 Sep 2013, Marek Polacek wrote: This patch adds the instrumentation of VLA bounds. Basically, it just checks that the size of a VLA is positive. I.e., We also issue an error if the size of the VLA is 0. It catches e.g. This is not an objection to this patch, but there are a few