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

Fix data types for ignored stat fields #401 #402

Merged
merged 1 commit into from Jul 27, 2021
Merged

Fix data types for ignored stat fields #401 #402

merged 1 commit into from Jul 27, 2021

Conversation

vykulakov
Copy link
Contributor

@vykulakov vykulakov commented Jul 27, 2021

Some ignored fields in the /proc/[pid]/stat file may have values that are bigger than the max value of the current Go type that is used for the ignored variable. That leads to issues in runtime on some systems that use the maximum possible values for the related fields.

Please note, that this PR fixes data types only for the ignored fields. Other fields in the stat file may be still affected especially on nodes that use 32-bit arch.

@pgier @discordianfish please have a look.

See for details:

Fixes: #401

Signed-off-by: Vyacheslav Kulakov kulakov.home@gmail.com

Some ignored fields in the /proc/[pid]/stat file may have values that are bigger than the max value of the current Go type that is used for the ignored variable. That leads to issues in runtime on some systems that use the maximum possible values for the related fields.

See for details:
* https://man7.org/linux/man-pages/man5/proc.5.html
* https://man7.org/linux/man-pages/man3/scanf.3.html

Signed-off-by: Vyacheslav Kulakov <kulakov.home@gmail.com>
@vykulakov
Copy link
Contributor Author

The interesting thing here is that the fixtures already have examples of the stat file that cannot pass tests but they. Unfortunately, haven't been used in the stat related tests.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM, nice find.

@vykulakov
Copy link
Contributor Author

vykulakov commented Jul 27, 2021

LGTM, nice find.

Should we deal with other fields then? Not in this PR of course.

I guess it will break compatibility but such change may be a good candidate for a release (1.x.x I mean).

@SuperQ
Copy link
Member

SuperQ commented Jul 27, 2021

This library is still pre 1.0. So, we can add breaking changes pretty easily.

@vykulakov
Copy link
Contributor Author

This library is still pre 1.0. So, we can add breaking changes pretty easily.

Please, check #403 and let me know if the mailing lists are a better place for such things. Thanks.

Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@discordianfish
Copy link
Member

@vykulakov github issues are best (unless it's help/usage questions etc), so thanks for filling!

@discordianfish discordianfish merged commit e979fa4 into prometheus:master Jul 27, 2021
@gdejong
Copy link

gdejong commented Jul 28, 2021

Thank you! Can you tell me if there is going to be a bugfix release for this?

@timja
Copy link

timja commented Aug 13, 2021

Hi I suspect this is causing a crash on startup for arm64 MacOS.

This is the only change that went into 1.2.2 of node_exporter it works on 1.2.1

see node_exporter issue prometheus/node_exporter#2120

Looks related to #403 as well, possibly fixed by #404 ?

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.

strconv.ParseInt: parsing "18446744073709551615": value out of range
5 participants