Hi Staale,
On 3/17/09 1:03 PM, Staale Smedseng wrote:
> #At file:///export/home/tmp/ss156133/z/b42502-51/ based on
> revid:azundris@stripped
>
> 2839 Staale Smedseng 2009-03-17
> Bug #42502 huge memory leak possible with timezone
> functions
>
> Unknown timezone specifications are properly rejected
> by the server, but are copied into tz_storage before
> rejection, and hence is retained until end of server
> life. With sufficiently large bogus timezone specs,
> it is easy to exhaust system memory.
>
> Allocation of memory for a copy of the timezone
> name is delayed until after verification of validity,
> at the cost of a memcpy of the timezone info (currently
> 88 bytes).
OK, shouldn't be a problem as it's only done once -- afterwards, the
entry is cached.
> modified:
> sql/tztime.cc
> === modified file 'sql/tztime.cc'
> --- a/sql/tztime.cc 2009-01-15 18:11:25 +0000
> +++ b/sql/tztime.cc 2009-03-17 16:03:53 +0000
> @@ -1825,23 +1825,14 @@ tz_load_from_open_tables(const String *t
> #ifdef ABBR_ARE_USED
> char chars[max(TZ_MAX_CHARS + 1, (2 * (MY_TZNAME_MAX + 1)))];
> #endif
> - DBUG_ENTER("tz_load_from_open_tables");
> -
> - /* Prepare tz_info for loading also let us make copy of time zone name */
> - if (!(alloc_buff= (char*) alloc_root(&tz_storage, sizeof(TIME_ZONE_INFO) +
> - tz_name->length() + 1)))
> - {
> - sql_print_error("Out of memory while loading time zone description");
> - return 0;
> - }
> - tz_info= (TIME_ZONE_INFO *)alloc_buff;
> - bzero(tz_info, sizeof(TIME_ZONE_INFO));
> - tz_name_buff= alloc_buff + sizeof(TIME_ZONE_INFO);
> - /*
> - By writing zero to the end we guarantee that we can call ptr()
> - instead of c_ptr() for time zone name.
> + /*
> + Used as a temporary tz_info until we decide that we actually want to
> + allocate and keep the tz info and tz name in tz_storage.
> */
> - strmake(tz_name_buff, tz_name->ptr(), tz_name->length());
> + TIME_ZONE_INFO tmp_tz_info;
> + bzero(&tmp_tz_info, sizeof(TIME_ZONE_INFO));
Take the opportunity and convert this bzero to a memset.
> + DBUG_ENTER("tz_load_from_open_tables");
>
> /*
> Let us find out time zone id by its name (there is only one index
> @@ -1866,7 +1857,8 @@ tz_load_from_open_tables(const String *t
> Most probably user has mistyped time zone name, so no need to bark here
> unless we need it for debugging.
> */
> - sql_print_error("Can't find description of time zone '%s'", tz_name_buff);
> + String tmp(*tz_name);
> + sql_print_error("Can't find description of time zone '%s'", tmp.c_ptr());
Use tz_name:
sql_print_error(".. zone '%.*s'", tz_name->length(), tz_name->ptr());
> #endif
> goto end;
> }
> @@ -1895,8 +1887,8 @@ tz_load_from_open_tables(const String *t
> /* If Uses_leap_seconds == 'Y' */
> if (table->field[1]->val_int() == 1)
> {
> - tz_info->leapcnt= tz_leapcnt;
> - tz_info->lsis= tz_lsis;
> + tmp_tz_info.leapcnt= tz_leapcnt;
> + tmp_tz_info.lsis= tz_lsis;
> }
>
> (void)table->file->ha_index_end();
> @@ -1932,18 +1924,18 @@ tz_load_from_open_tables(const String *t
> #ifdef ABBR_ARE_USED
> // FIXME should we do something with duplicates here ?
> table->field[4]->val_str(&abbr,&abbr);
> - if (tz_info->charcnt + abbr.length() + 1> sizeof(chars))
> + if (tmp_tz_info.charcnt + abbr.length() + 1> sizeof(chars))
> {
> sql_print_error("Error while loading time zone description from "
> "mysql.time_zone_transition_type table: not enough "
> "room for abbreviations");
> goto end;
> }
> - ttis[ttid].tt_abbrind= tz_info->charcnt;
> - memcpy(chars + tz_info->charcnt, abbr.ptr(), abbr.length());
> - tz_info->charcnt+= abbr.length();
> - chars[tz_info->charcnt]= 0;
> - tz_info->charcnt++;
> + ttis[ttid].tt_abbrind= tmp_tz_info.charcnt;
> + memcpy(chars + tmp_tz_info.charcnt, abbr.ptr(), abbr.length());
> + tmp_tz_info.charcnt+= abbr.length();
> + chars[tmp_tz_info.charcnt]= 0;
> + tmp_tz_info.charcnt++;
>
> DBUG_PRINT("info",
> ("time_zone_transition_type table: tz_id=%u tt_id=%u tt_gmtoff=%ld "
> @@ -1956,9 +1948,9 @@ tz_load_from_open_tables(const String *t
> #endif
>
> /* ttid is increasing because we are reading using index */
> - DBUG_ASSERT(ttid>= tz_info->typecnt);
> + DBUG_ASSERT(ttid>= tmp_tz_info.typecnt);
>
> - tz_info->typecnt= ttid + 1;
> + tmp_tz_info.typecnt= ttid + 1;
>
> res= table->file->index_next_same(table->record[0],
> table->field[0]->ptr, 4);
> @@ -1990,14 +1982,14 @@ tz_load_from_open_tables(const String *t
> ttime= (my_time_t)table->field[1]->val_int();
> ttid= (uint)table->field[2]->val_int();
>
> - if (tz_info->timecnt + 1> TZ_MAX_TIMES)
> + if (tmp_tz_info.timecnt + 1> TZ_MAX_TIMES)
> {
> sql_print_error("Error while loading time zone description from "
> "mysql.time_zone_transition table: "
> "too much transitions");
> goto end;
> }
> - if (ttid + 1> tz_info->typecnt)
> + if (ttid + 1> tmp_tz_info.typecnt)
> {
> sql_print_error("Error while loading time zone description from "
> "mysql.time_zone_transition table: "
> @@ -2005,9 +1997,9 @@ tz_load_from_open_tables(const String *t
> goto end;
> }
>
> - ats[tz_info->timecnt]= ttime;
> - types[tz_info->timecnt]= ttid;
> - tz_info->timecnt++;
> + ats[tmp_tz_info.timecnt]= ttime;
> + types[tmp_tz_info.timecnt]= ttid;
> + tmp_tz_info.timecnt++;
>
> DBUG_PRINT("info",
> ("time_zone_transition table: tz_id: %u tt_time: %lu tt_id: %u",
> @@ -2032,6 +2024,34 @@ tz_load_from_open_tables(const String *t
> table= 0;
>
> /*
> + Let us check how correct our time zone description is. We don't check for
> + tz->timecnt< 1 since it is ok for GMT.
> + */
> + if (tmp_tz_info.typecnt< 1)
> + {
> + sql_print_error("loading time zone without transition types");
> + goto end;
> + }
> +
> + /* Allocate memory for the timezone info and timezone name in tz_storage. */
> + if (!(alloc_buff= (char*) alloc_root(&tz_storage, sizeof(TIME_ZONE_INFO) +
> + tz_name->length() + 1)))
> + {
> + sql_print_error("Out of memory while loading time zone description");
> + return 0;
> + }
> +
> + /* Move the temporary tz_info into the allocated area */
> + tz_info= (TIME_ZONE_INFO *)alloc_buff;
> + memcpy(tz_info,&tmp_tz_info, sizeof(TIME_ZONE_INFO));
> + tz_name_buff= alloc_buff + sizeof(TIME_ZONE_INFO);
> + /*
> + By writing zero to the end we guarantee that we can call ptr()
> + instead of c_ptr() for time zone name.
> + */
> + strmake(tz_name_buff, tz_name->ptr(), tz_name->length());
> +
> + /*
> Now we will allocate memory and init TIME_ZONE_INFO structure.
> */
> if (!(alloc_buff= (char*) alloc_root(&tz_storage,
> @@ -2062,15 +2082,7 @@ tz_load_from_open_tables(const String *t
> tz_info->ttis= (TRAN_TYPE_INFO *)alloc_buff;
> memcpy(tz_info->ttis, ttis, tz_info->typecnt * sizeof(TRAN_TYPE_INFO));
>
> - /*
> - Let us check how correct our time zone description and build
> - reversed map. We don't check for tz->timecnt< 1 since it ok for GMT.
> - */
> - if (tz_info->typecnt< 1)
> - {
> - sql_print_error("loading time zone without transition types");
> - goto end;
> - }
> + /* Build reversed map. */
> if (prepare_tz_info(tz_info,&tz_storage))
> {
> sql_print_error("Unable to build mktime map for time zone");
>
Looks good, approved.
Regards,
-- Davi Arnaut
