Bug #9652
closedMarshal doesn't dump/load Time#zone correctly with too many time object
Description
Hi, there
I wrote a script to test Marshal dump/load with many time object ( see attachment ).
basic idea is:
- A is container of time, it contains 100,000 time instance (all Time.now).
- 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
Updated by normalperson (Eric Wong) over 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) over 10 years ago
Seems better to call rb_str_new_frozen()
before setting vtm.zone
.
Updated by normalperson (Eric Wong) over 10 years ago
nobu@ruby-lang.org wrote:
Seems better to call
rb_str_new_frozen()
before settingvtm.zone
.
OK. Though rb_fstring() might be better because time objects
with identical zones are common.
Updated by normalperson (Eric Wong) over 10 years ago
normalperson@yhbt.net wrote:
nobu@ruby-lang.org wrote:
Seems better to call
rb_str_new_frozen()
before settingvtm.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.
Updated by Anonymous over 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) over 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) over 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) over 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) over 10 years 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.
Updated by usa (Usaku NAKAMURA) over 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) over 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) about 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.