Luis, I have committed your patch with some changes or missing parts. Thanks for the contribuitins. Some of the comments were due to a more detailed review while merging your code in.
Index: mono/metadata/object.c =================================================================== --- mono/metadata/object.c (revision 17) +++ mono/metadata/object.c (working copy) @@ -3513,12 +3513,23 @@ } /* helper macros to check for overflow when calculating the size of arrays */ -#define MYGUINT32_MAX 4294967295U +#if (GLIB_SIZEOF_SIZE_T < 4 ) I changed the check to be against MONO_BIG_ARRAYS. @@ -3548,34 +3559,32 @@ /* A single dimensional array with a 0 lower bound is the same as an szarray */ .... if (CHECK_ADD_OVERFLOW_UN (byte_len, 3)) - mono_gc_out_of_memory (MYGUINT32_MAX); + mono_gc_out_of_memory (MONO_ARRAY_MAX_SIZE); byte_len = (byte_len + 3) & ~3; You might need to adjust this alignment check to be aware of your changes. =================================================================== --- mono/metadata/icall.c (revision 17) +++ mono/metadata/icall.c (working copy) @@ -527,11 +527,11 @@ aklass = mono_bounded_array_class_get (mono_class_from_mono_type (type->type), mono_array_length (lengths), bounded); - sizes = alloca (aklass->rank * sizeof(guint32) * 2); + sizes = alloca (aklass->rank * sizeof(mono_array_size_t) * 2); for (i = 0; i < aklass->rank; ++i) { - sizes [i] = mono_array_get (lengths, guint32, i); + sizes [i] = mono_array_get (lengths, gint32, i); if (bounds) - sizes [i + aklass->rank] = mono_array_get (bounds, guint32, i); + sizes [i + aklass->rank] = mono_array_get (bounds, gint32, i); else sizes [i + aklass->rank] = 0; } Why are you using mono_array_get with gint32? I can't think of a reason for this change, as it won't matter for mono_array_new_full. Besides that, this is asking for trouble as you are mixing signed and unsigned numbers. @@ -560,6 +606,27 @@ mono_raise_exception (mono_get_exception_index_out_of_range ()); if (this->bounds == NULL) + length = this->max_length; + else + length = this->bounds [dimension].length; + + if (length > G_MAXINT32) + mono_raise_exception (mono_get_exception_overflow ()); + + return length; +} I have put the length check inside a #ifdef MONO_BIG_ARRAYS @@ -541,6 +541,51 @@ return array; } +static MonoArray * +ves_icall_System_Array_CreateInstanceImpl64 (MonoReflectionType *type, MonoArray *lengths, MonoArray *bounds) ... + for (i = 0; i < mono_array_length (lengths); i++) + if ((mono_array_get (lengths, gint64, i) < 0) || + (mono_array_get (lengths, gint64, i) > MONO_ARRAY_MAX_INDEX)) + mono_raise_exception (mono_get_exception_argument_out_of_range (NULL)); Can't this expression be simplified to "if ((mono_array_get (lengths, guint64, i) > MONO_ARRAY_MAX_INDEX)", note we are reading it as an unsigned value. ... + sizes = alloca (aklass->rank * sizeof(mono_array_size_t) * 2); + for (i = 0; i < aklass->rank; ++i) { + sizes [i] = mono_array_get (lengths, gint64, i); + if (bounds) + sizes [i + aklass->rank] = mono_array_get (bounds, gint64, i); + else + sizes [i + aklass->rank] = 0; + } Note that sizes element type is an unsigned number, so we must read it acordingly and cast it to mono_array_size_t to ajust for the desired size - and avoid warnings as well. We can use the same code for the bound check I suggested for both CreateInstanceImpl versions. If we do that we could fix the ugly aspect of having two 99% identical functions by defining it as a MACRO and expanding it twice. On Fri, May 23, 2008 at 7:10 PM, Luis F. Ortiz <[EMAIL PROTECTED]> wrote: > Rodrigo: > Sorry for the delayed response, things got busy, and I did not notice > your message. > Please go ahead and check in the parts that you feel most comfortable with, > since I do not > have commit rights. I'll adapt any further submissions against the next > official cut. > > The changes I have made are to be released under the X11 license and I > vouch that > I have talked to the appropriate people at my employer (Interactive > Supercomputing) and > they agree as well to release this and any further contributions I make to > Mono while > under their employ under the X11 license. There, that ought to make > everybody happy. > > Luis F. Ortiz > > > On May 16, 2008, at 10:53 AM, Rodrigo Kumpera wrote:\ > > Hi Luis, > > Most parts of your patch are ok to be commited. As I said before, it's > better to have separate patches to speed up the review. As you get more > changes in, it will ease your work on preparing patches and will ease on > tracking head changes. > > Right now I'm ok with the configure, mono_array_t and related changes. > These were easy to review and don't introduce any change in behavior, so we > can check-in then right now. The others requires more testing from me and > would be easier to both of us if done after. > Do you mind cooking you a patch only with the ok parts. > > Remember that you need to either release these changes under the X11 > license or have done some copyright assignment paperwork. > > Thanks, > Rodrigo > > > > On Thu, May 8, 2008 at 8:21 PM, Luis F. Ortiz <[EMAIL PROTECTED]> > wrote: > >> On May 8, 2008, at 9:29 AM, Rodrigo Kumpera wrote: >> >>> One important thing I forgot. If you break your patch into a few smaller >>> ones the review process will be a lot easier to every one involved. >>> The first one can introduce new types and configuration foo; then other >>> to fix codegen and Array methods and; at last, a bunch of fixes to classlib >>> issues -like sockets, file i/o and so on. >>> >>> Cheers, >>> Rodrigo >>> >> >> OK, here is another stab at the changes. >> >> This modifies the following files: >> configure.in >> mono/metadata/object.c >> mono/metadata/object.h >> mono/metadata/icall-def.h >> mono/metadata/icall.c >> mono/metadata/socket-io.c >> >> These files: >> 1) Add a new configuration option, --enable-big-arrays, which defines >> conditionally defines MONO_BIG_ARRAYS in config.h, >> 2) Add in mono/metadata/object.h : >> A) A new typedef for mono_array_size_t to be either guint32 or guint64 >> B) A new #define for MONO_ARRAY_MAX_INDEX for the biggest valid array >> index, i.e. G_MAXINTxx >> C) A new #define for MONO_ARRAY_MAX_SIZE for the biggest valid array >> allocation, i.e. G_MAXUINTxx >> D) The above all controlled by MONO_BIG_ARRAYS >> 3) Modify the definitions of mono_array_new(), mono_array_new_full(), and >> mono_array_new_specific() to match, >> 4) Modify the usages of those functions in the metadata directory to >> match, >> 5) Add range checking in ves_icall_System_Array_CreateInstanceImpl64 so >> it works with or without MONO_BIG_ARRAYS, >> 6) Attempt to address all the mistakes you pointed out. >> >> These changes, in addition to the other changes I made, pass "make check" >> with the exception of Tests.test_0_regress_75832(), as previously confessed. >> >> >> >> >> >> /Ortiz/Luis >> >> >> > >
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list