Project

General

Profile

Actions

Bug #9652

closed

Marshal doesn't dump/load Time#zone correctly with too many time object

Added by Cantin (Cantin Xu) about 10 years ago. Updated almost 10 years ago.

Status:
Closed
Assignee:
-
Target version:
ruby -v:
ruby 2.1.1p76 (2014-02-24 revision 45161) [x86_64-darwin13.0]
[ruby-core:61584]

Description

Hi, there

I wrote a script to test Marshal dump/load with many time object ( see attachment ).

basic idea is:

  1. A is container of time, it contains 100,000 time instance (all Time.now).
  2. dump/load A by using Marshal, expected behavior is all the time zone must same as Time.now.zone

but I get different things or unreadable code in some Time instance by calling time.zone.
mostly it's "UTF-8'.

env: Mac 10.9.2, ruby: ruby 2.1.1-p76

PS: it's fine while A contain 10,000 time instance


Files

timezone_test.rb (409 Bytes) timezone_test.rb Cantin (Cantin Xu), 03/19/2014 06:51 AM

Updated by normalperson (Eric Wong) about 10 years ago

I cannot reproduce the problem here, however it could be the
lack of reference for GC.

Can you add this rb_ivar_set call?

--- a/time.c
+++ b/time.c
@@ -4828,6 +4828,7 @@ end_submicro: ;
     }
     if (!NIL_P(zone)) {
 	tobj->vtm.zone = RSTRING_PTR(zone);
+	rb_ivar_set(str, id_zone, zone);
     }
 
     return time;

Also at: git://80x24.org/ruby.git time-load-zone
http://bogomips.org/ruby.git/patch?id=9418df96145

Updated by nobu (Nobuyoshi Nakada) about 10 years ago

Seems better to call rb_str_new_frozen() before setting vtm.zone.

Updated by normalperson (Eric Wong) about 10 years ago

wrote:

Seems better to call rb_str_new_frozen() before setting vtm.zone.

OK. Though rb_fstring() might be better because time objects
with identical zones are common.

Updated by normalperson (Eric Wong) about 10 years ago

wrote:

wrote:

Seems better to call rb_str_new_frozen() before setting vtm.zone.

OK. Though rb_fstring() might be better because time objects
with identical zones are common.

Or not, don't want potential untrusted sources populating the
fstring table; either. Will commit w/ str_new_frozen + test case.

Actions #5

Updated by Anonymous about 10 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

Applied in changeset r45364.


time.c: freeze and preserve marshal-loaded time zone

We need to prevent vtm.zone from pointing to a GC-ed string buffer.
The rb_copy_generic_ivar call misses it because get_attr deleted it.
Thanks to nobu for the rb_str_new_frozen suggestion.

  • time.c (time_mload): freeze and preserve marshal-loaded time zone
  • test/ruby/test_time.rb: add test for GC on loaded object
    [Bug #9652]

Updated by normalperson (Eric Wong) about 10 years ago

Thanks all, committed as r45364
Sorry my original patch set the ivar on the wrong object :x
But fortunately I was able to reproduce the GC error and test
for it.

Updated by nagachika (Tomoyuki Chikanaga) about 10 years ago

  • Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN to 2.0.0: REQUIRED, 2.1: REQUIRED

Updated by usa (Usaku NAKAMURA) about 10 years ago

r45364 introduced a test failure on Windows, and r45395 didn't fix it.
After all, there is no portabl time zone name, I guess.
Doesn't someone think of a better test?

Updated by normalperson (Eric Wong) about 10 years ago

wrote:

r45364 introduced a test failure on Windows, and r45395 didn't fix it.
After all, there is no portabl time zone name, I guess.
Doesn't someone think of a better test?

Sorry about the portability problems. r45403 works for me on a GNU system.
If all else fails, maybe only enable TZ setting tests on a few platforms
like test/ruby/test_time_tz.rb does.

Updated by usa (Usaku NAKAMURA) about 10 years ago

Eric Wong wrote:

Sorry about the portability problems.

Of course you don't have to take it seriously.
It's the reason why platform maintainers exist.

I'm worried that I don't understand the intention of the test correctly.
If there are any problems, please correct.

Updated by usa (Usaku NAKAMURA) almost 10 years ago

  • Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED to 2.0.0: DONE, 2.1: REQUIRED

backported r45364, r45395, r45396, r45403 and r45406 into ruby_2_0_0 at r45752.

Updated by nagachika (Tomoyuki Chikanaga) almost 10 years ago

  • Backport changed from 2.0.0: DONE, 2.1: REQUIRED to 2.0.0: DONE, 2.1: DONE

r45364, r45395, r45396, r45403 and r45406 were backported into ruby_2_1 branch at r46304.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0