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

Check overflows and validate timestamp9 value range. #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

higuoxing
Copy link
Contributor

@higuoxing higuoxing commented Mar 1, 2023

This patch helps add checks for potential overflows when converting other date time types to timestamp9 and adds checks for validating then timestamp9 values range [1900-01-01 00:00:00.000000000 UTC+0, 2262-01-01 00:00:00.000000000 UTC+0).

Trying to fix #12

P.S. I don't know if the value range is appropriate, let's discuss about it?

This patch helps add checks for potential overflows when converting
other date time types to timestamp9 and adds checks for validating then
timestamp9 values range [1900-01-01 00:00:00.000000000 UTC+0, 2262-01-01
00:00:00.000000000 UTC+0).
@fvannee
Copy link
Collaborator

fvannee commented Mar 2, 2023

Thanks for the submission. I'm open to having better checks here to prevent cases like the ones mentioned in #12 . Did you pick the current min/max range arbitrarily? I think the range is well suited for most cases, but maybe it's too restrictive. Would it be difficult to support the full range of bigint from minimum value to maximum value or do you think it'd be too complicated with overflow checks?

Edit: Ah I see this may be quite complex in the addition with interval checks

@fvannee
Copy link
Collaborator

fvannee commented Mar 2, 2023

timestamp9_recv would need a check added too, otherwise invalid values can be constructed from the binary libpq protocol

- Validate timestamp9 value in timestamp9_recv().
- Enlarge the timestamp9 value range to '[1700-01-01 UTC+0, 2262-01-01
  UTC+0)'.
@higuoxing
Copy link
Contributor Author

Did you pick the current min/max range arbitrarily? I think the range is well suited for most cases, but maybe it's too restrictive. Would it be difficult to support the full range of bigint from minimum value to maximum value or do you think it'd be too complicated with overflow checks?

Yes, I pick the min/max year arbitrarily, but pick the date 01-01 deliberately, since it's easy for validating the timestamp and that's what PostgreSQL do for the builtin timestamp range.

Based on your comment, I enlarge the value range to [1700-01-01 00:00:00.000000000 UTC+0, 2262-01-01 00:00:00.000000000 UTC+0) in my latest patch, because INT64_MIN corresponds to 1677-09-21 00:12:43.145224192 +0000 and INT64_MAX corresponds to 2262-04-11 23:47:16.854775807 +0000.

timestamp9_recv would need a check added too, otherwise invalid values can be constructed from the binary libpq protocol

Good catch! Thanks for pointing it out.

@higuoxing
Copy link
Contributor Author

In my latest commit, I modified timestamp9.sql since we need to validate the range when doing the conversion from bigint to timestamp9.

@asiayeah
Copy link

I don't see it merged into 1.4.0.

Any plan to merge and release this PR? Thank you.

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.

Validate timestamp value when doing conversions.
3 participants