Bug #9652

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

Added by Cantin Xu over 1 year ago. Updated about 1 year ago.

[ruby-core:61584]
Status:Closed
Priority:Normal
Assignee:-
ruby -v:ruby 2.1.1p76 (2014-02-24 revision 45161) [x86_64-darwin13.0] Backport:2.0.0: DONE, 2.1: DONE

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

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

Associated revisions

Revision 45364
Added by normal over 1 year ago

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]

Revision 45364
Added by normal over 1 year ago

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]

Revision 45395
Added by Yui NARUSE over 1 year ago

Asia/Tokyo is more portable value for ENV["TZ"] [Bug #9652]

FreeBSD doesn't accept "Japan".

Revision 45395
Added by Yui NARUSE over 1 year ago

Asia/Tokyo is more portable value for ENV["TZ"] [Bug #9652]

FreeBSD doesn't accept "Japan".

Revision 45403
Added by Usaku NAKAMURA over 1 year ago

  • test/ruby/test_time.rb: use portable time zone. see [Bug #9652]

Revision 45403
Added by Usaku NAKAMURA over 1 year ago

  • test/ruby/test_time.rb: use portable time zone. see [Bug #9652]

Revision 45752
Added by Usaku NAKAMURA over 1 year ago

merge revision(s) 45364,45395,45396,45403,45406: [Backport #9652]

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

Revision 46304
Added by Tomoyuki Chikanaga about 1 year ago

merge revision(s) r45364,r45395,r45396,r45403,r45406: [Backport #9652]

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

History

#1 Updated by Eric Wong over 1 year 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

#2 Updated by Nobuyoshi Nakada over 1 year ago

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

#3 Updated by Eric Wong over 1 year ago

nobu@ruby-lang.org 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.

#4 Updated by Eric Wong over 1 year ago

normalperson@yhbt.net wrote:

nobu@ruby-lang.org 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.

#5 Updated by Anonymous over 1 year 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]

#6 Updated by Eric Wong over 1 year 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.

#7 Updated by Tomoyuki Chikanaga over 1 year ago

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

#8 Updated by Usaku NAKAMURA over 1 year 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?

#9 Updated by Eric Wong over 1 year ago

usa@garbagecollect.jp 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.

#10 Updated by Usaku NAKAMURA over 1 year 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.

#11 Updated by Usaku NAKAMURA over 1 year 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.

#12 Updated by Tomoyuki Chikanaga about 1 year 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.

Also available in: Atom PDF