Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove constCast in Timezone.deinit #36

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Remove constCast in Timezone.deinit #36

merged 2 commits into from
Oct 28, 2024

Conversation

Ratakor
Copy link
Contributor

@Ratakor Ratakor commented Oct 27, 2024

Closes #32

create a Timezone, set it to a Datetime, de-initialize it, and overwrite it with a new one

I think that it is fine to say that this is undefined behavior since Datetime takes a pointer to a Timezone so it implicitely means that Timezone should live as long as Datetime for it to be correct.

Also, having const Timezone doesn't prevent this footgun: create a Timezone, set it to a Datetime, and de-initialize. This result in a wrong offset too.


This change also prevents from doing

zdt.Timezone.UTC.deinit();

which I don't know why someone would do that but it will result in a segfault with current behavior.

@FObersteiner
Copy link
Owner

FObersteiner commented Oct 28, 2024

I think that it is fine to say that this is undefined behavior since Datetime takes a pointer to a Timezone so it implicitely means that Timezone should live as long as Datetime for it to be correct.

Right, good point! So let's go with the Timezone being a 'var'. Since deinit modifies it, this is more idiomatic IMHO.


This result in a wrong offset too.

are you sure? To my understanding, de-initializing the tz should not alter the offset; added a check to

test "tz deinit is mem-safe" {

@FObersteiner FObersteiner changed the base branch from master to dev October 28, 2024 06:42
@FObersteiner FObersteiner merged commit dcade9c into FObersteiner:dev Oct 28, 2024
FObersteiner added a commit that referenced this pull request Oct 28, 2024
* prepare next version

* Remove constCast in Timezone.deinit (#36)

* ensure UTC can be de-inited safely

* update code comments / docs

* rename runtimeFromTzfile to fromSystemTzdata

* update readme and changelog

* update changelog

* update test

---------

Co-authored-by: Ratakor <[email protected]>
@Ratakor
Copy link
Contributor Author

Ratakor commented Oct 30, 2024

I got another idea to make Timezone constant while preventing from freeing UTC, it's basically to remove the clearing of __name_data and tzinfo which is fine since using something deinitialized is almost always an undefined behavior. I don't know if it's worth making another PR about the same subject so you can check the changes here master...Ratakor:zdt:master.

@FObersteiner
Copy link
Owner

tz deinit
In pub fn deinit(tz: Timezone) void {, I wonder if tz is actually passed by reference under the hood, because at the moment, the Zig compiler may decide to do so if you don't explicitly specify pass-by-pointer [docs]. Put the other way around, if tz was passed by value here, i.e. copied, deinit should have no effect outside of the function, no?
I'm not sure if this applies here, but I know that there has been quite some discussion (e.g. here) around it because it can lead to incorrect results.

POSIX TZ
I saw you added a posix.zig file :) Was just thinking how to proceed with this, it's a feature I would appreciate to have in the lib, however I know from a contribution to https://github.com/leroycep/zig-tzif that it's a bit of a hassle to write a good parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation Fault when using Datetime after deinitializing a Timezone
2 participants