maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

On May 10, Michael Widenius wrote:
> > > diff --git a/sql/item.cc b/sql/item.cc
> > > index 1a86b8b3114..0f160fa257b 100644
> > > --- a/sql/item.cc
> > > +++ b/sql/item.cc
> > > @@ -55,7 +55,8 @@ const char *item_empty_name="";
> > >  const char *item_used_name= "\0";
> > >
> > >  static int save_field_in_field(Field *, bool *, Field *, bool);
> > > -
> > > +const Item_bool_static Item_false("FALSE", 0);
> > > +const Item_bool_static Item_true("TRUE", 1);
> >
> > The main question - we have tons of modifiable Item members. How do
> > you know none of them will modified in your Item_bool_static
> > objects?
> 
> Because these are global const. We will get an assert if anyone tries.

No. It doesn't go into a read-only segment.
Here's the patch that really makes it to go there:

--- a/sql/item.h
+++ b/sql/item.h
@@ -4395,7 +4395,22 @@ class Item_bool_static :public Item_bool
   { DBUG_ASSERT(0); }
 };
 
-extern const Item_bool_static Item_false, Item_true;
+class Item_bool_static_true: public Item_bool_static
+{
+  public:
+  Item_bool_static_true():
+    Item_bool_static("TRUE", 1) {}
+};
+
+class Item_bool_static_false: public Item_bool_static
+{
+  public:
+  Item_bool_static_false():
+    Item_bool_static("FALSE", 0) {}
+};
+
+extern const Item_bool_static_true Item_true;
+extern const Item_bool_static_false Item_false;
 
 class Item_uint :public Item_int
 {

diff --git a/sql/item.cc b/sql/item.cc
index 67fcb7d4e0c..1f0031dcfb9 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -55,8 +55,9 @@ const char *item_empty_name="";
 const char *item_used_name= "\0";
 
 static int save_field_in_field(Field *, bool *, Field *, bool);
-const Item_bool_static Item_false("FALSE", 0);
-const Item_bool_static Item_true("TRUE", 1);
+
+const Item_bool_static_false Item_false;
+const Item_bool_static_true Item_true __attribute__ ((section (".rodata")));
 
 /**
   Compare two Items for List<Item>::add_unique()
=============================================================

With this patch the server immediately crashes on startup, on

3052    class Type_numeric_attributes
3053    {
...  
3069      Type_numeric_attributes()
3070 =>  :max_length(0), decimals(0), unsigned_flag(false)
3071      { }

with the stack trace:

#0  0x0000555555fa7210 in Type_numeric_attributes::Type_numeric_attributes (
    this=0x555556db0468 <Item_true+8>) at sql/sql_type.h:3070
#1  0x0000555555fa7244 in Type_std_attributes::Type_std_attributes (
    this=0x555556db0468 <Item_true+8>) at sql/sql_type.h:3128
#2  0x0000555555fa7282 in Type_all_attributes::Type_all_attributes (
    this=0x555556db0460 <Item_true>) at sql/sql_type.h:3305
#3  0x0000555556214d70 in Item::Item (this=0x555556db0460 <Item_true>)
    at sql/item.cc:452
#4  0x0000555556236efa in Item_basic_value::Item_basic_value (
    this=0x555556db0460 <Item_true>) at sql/item.h:2929
#5  0x0000555556236f4e in Item_basic_constant::Item_basic_constant (
    this=0x555556db0460 <Item_true>) at sql/item.h:2959
#6  0x000055555623725e in Item_literal::Item_literal (
    this=0x555556db0460 <Item_true>) at sql/item.h:3338
#7  0x00005555562372a2 in Item_num::Item_num (this=0x555556db0460 <Item_true>)
    at sql/item.h:3353
#8  0x0000555556237d74 in Item_int::Item_int (this=0x555556db0460 <Item_true>,
    str_arg=0x555556dafc3f "TRUE", i=1, length=1) at sql/item.h:4330
#9  0x0000555556237e2b in Item_bool::Item_bool (
    this=0x555556db0460 <Item_true>, str_arg=0x555556dafc3f "TRUE", i=1)
    at sql/item.h:4371
#10 0x0000555556237f3d in Item_bool_static::Item_bool_static (
    this=0x555556db0460 <Item_true>, str_arg=0x555556dafc3f "TRUE", i=1)
    at sql/item.h:4392
#11 0x000055555623805c in Item_bool_static_true::Item_bool_static_true (
    this=0x555556db0460 <Item_true>) at sql/item.h:4402
#12 0x00005555562361be in __static_initialization_and_destruction_0 (
    __initialize_p=1, __priority=65535) at sql/item.cc:60
#13 0x00005555562361f4 in _GLOBAL__sub_I_item.cc(void) () at sql/item.cc:10755
#14 0x0000555556b8cf45 in __libc_csu_init (argc=argc@entry=1,
    argv=argv@entry=0x7fffffffdeb8, envp=0x7fffffffdec8) at elf-init.c:89
#15 0x00007ffff76ebbcc in __libc_start_main (main=
    0x555555d2c4b5 <main(int, char**)>, argc=1, argv=0x7fffffffdeb8, init=
    0x555556b8cf00 <__libc_csu_init>, fini=<optimized out>,
    rtld_fini=<optimized out>, stack_end=0x7fffffffdea8)
    at ../csu/libc-start.c:270
#16 0x0000555555d2c3fa in _start ()

that is, before main(), on attempt to initialize your Item_true.

In other words, you'll never be able to put it into a read-only segment.
And you have no protection against run-time modifications of your static
items. So, now I see it as a very dangerous trick and I ask you not to
push it.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx