maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

Hi,

I'd like to report my findings during the investigation of MDEV-7912, ask
for a review on my patch and see your opinion on the status of the related
code. The patch and all the issues that I am raising are for the 5.5 branch.

With the attached patch, the server crash is eliminated. The problem we had
was caused by casting a 64bit unsigned int to a 32bit unsigned int,
representing a number of bytes to allocate. When the ranges were too big,
it caused an overflow and we allocated too little space.

On a related note, while looking at the code in the uniques.cc file, I've
discovered a lot of potential issues, that are almost guaranteed to fail,
if we actually do use more than 4 GB of memory. This holds true especially
for Windows, because ulong is 32 bits for a 64 bit compilation, while on
Linux, it is 64 bit.
I've tried to convert most of the code that made use of uint or ulong or
ulonglong for address space indexing (arrays, indices in arrays, memory
sizes) to size_t, so that we escape the portability problem when compiling
for 32 bit or 64 bit. Unfortunately, the task branches out into various
other files that make use of those values. An example of a such a problem
is in Unique::Unique constructor, line 90, where init_tree will get passed
an overflown parameter (max_in_memory_size). It just so happens that there
is a guard within the init_tree function that sets a minimum value, in case
we do happen to pass an overflown value. This is however a lucky occurrence.

Unique::get_use_cost can output a bad cost value, because
get_merge_many_buffs_cost casts the same ulonglong memory size parameter to
a ulong.
There are many examples like this.

I'd like to get your input on this issue, is this something we plan to fix
in the near future? Should I embark on the journey of slowly fixing at
least parts of it? Also, this issue seems to plague much of the code base,
as I get a huge wall of similar size_t -> uint32 warnings across most files.

Regards,
Vicențiu

commit 40d6d95824ffdd53e0dcdf03ce7ef57cb28e1574
Author: Vicentiu Ciorbaru <vicentiu@xxxxxxxxxxx>
Date:   Fri Apr 24 13:47:16 2015 +0300

    MDEV-7912 multitable delete with wrongly set sort_buffer_size crashes in merge_buffers
    
    Fixed overflow error that caused fewer bites to be allocated than
    necessary on Windows 64 bit. This is due to ulong being 32 bit on
    64 bit Windows and 64 bit on 64 bit Linux.

diff --git a/sql/uniques.cc b/sql/uniques.cc
index 72411be..b9436ea 100644
--- a/sql/uniques.cc
+++ b/sql/uniques.cc
@@ -97,7 +97,7 @@ int unique_intersect_write_to_ptrs(uchar* key, element_count count, Unique *uniq
   max_elements= (ulong) (max_in_memory_size /
                          ALIGN_SIZE(sizeof(TREE_ELEMENT)+size));
   (void) open_cached_file(&file, mysql_tmpdir,TEMP_PREFIX, DISK_BUFFER_SIZE,
-		   MYF(MY_WME));
+                          MYF(MY_WME));
 }
 
 
@@ -607,8 +607,8 @@ bool Unique::walk(TABLE *table, tree_walk_action action, void *walk_action_arg)
     return 1;
   if (flush_io_cache(&file) || reinit_io_cache(&file, READ_CACHE, 0L, 0, 0))
     return 1;
-  ulong buff_sz= (max_in_memory_size / full_size + 1) * full_size;
-  if (!(merge_buffer= (uchar *) my_malloc((ulong) buff_sz, MYF(0))))
+  size_t buff_sz= (max_in_memory_size / full_size + 1) * full_size;
+  if (!(merge_buffer= (uchar *) my_malloc(buff_sz, MYF(0))))
     return 1;
   if (buff_sz < (ulong) (full_size * (file_ptrs.elements + 1)))
     res= merge(table, merge_buffer, buff_sz >= full_size * MERGEBUFF2) ;
@@ -738,8 +738,8 @@ bool Unique::get(TABLE *table)
   if (flush())
     return 1;
   
-  ulong buff_sz= (max_in_memory_size / full_size + 1) * full_size;
-  if (!(sort_buffer= (uchar*) my_malloc(buff_sz, MYF(0))))
+  size_t buff_sz= (max_in_memory_size / full_size + 1) * full_size;
+  if (!(sort_buffer= (uchar*) my_malloc(buff_sz, MYF(MY_WME))))
     return 1;
 
   if (merge(table, sort_buffer, FALSE))