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

• Status: Closed
• Assignee: -
• Target version: 2.2.0
• ruby -v: ruby 2.1.1p76 (2014-02-24 revision 45161) [x86_64-darwin13.0]
• Backport: 2.0.0: DONE, 2.1: DONE

[Cantin (Cantin Xu)]

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

[normalperson (Eric Wong)]

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

[nobu (Nobuyoshi Nakada)]

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

[normalperson (Eric Wong)]

[e-mail] 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.

[normalperson (Eric Wong)]

[e-mail] wrote:

[e-mail] 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.

• 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]

[normalperson (Eric Wong)]

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.

[nagachika (Tomoyuki Chikanaga)]

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

[usa (Usaku NAKAMURA)]

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?

[normalperson (Eric Wong)]

[e-mail] 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.

[usa (Usaku NAKAMURA)]

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.

[usa (Usaku NAKAMURA)]

• 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.

[nagachika (Tomoyuki Chikanaga)]

• 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.