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

RetentionConfig: restrict days to uint16 #824

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Oct 10, 2024

As discussed in #821, allowing huge retention values - as the type uint64 allows - may result in overflows. Especially Go's time.AddDate() method silently overflows for huge values, resulting in unexpected comparison times.

$ ./icingadb --config <(echo 'retention: {history-days: -1}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint16 ( overflow )

$ ./icingadb --config <(echo 'retention: {history-days: 100000}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal 100000 into Go value of type uint16 ( overflow )

As discussed in #821, allowing huge retention values - as the type
uint64 allows - may result in overflows. Especially Go's time.AddDate()
method silently overflows for huge values, resulting in unexpected
comparison times.

> $ ./icingadb --config <(echo 'retention: {history-days: -1}')
> can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint16 ( overflow )
>
> $ ./icingadb --config <(echo 'retention: {history-days: 100000}')
> can't parse YAML file /proc/self/fd/11: cannot unmarshal 100000 into Go value of type uint16 ( overflow )
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$2^{16}$ days is almost 180 years, so that range should be more than enough for all practical use (uint8 with less than a year would be too small). On the other hand, now - 180 years, i.e. the year 1844 is within time.Time bounds and no problem for time.AddDate().

@lippserd lippserd merged commit a6d134c into main Oct 10, 2024
32 checks passed
@lippserd lippserd deleted the retention-days-uint16 branch October 10, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants