Correct buffer_bytes > INT_MAX on BSD/amd64.#712
Correct buffer_bytes > INT_MAX on BSD/amd64.#712discordianfish merged 6 commits intoprometheus:masterfrom
Conversation
The sysctl vfs.bufspace returns either an int or a long, depending on the value. Large values of vfs.bufspace will result in error messages like: couldn't get meminfo: cannot allocate memory This will detect the returned data type, and cast appropriately.
collector/sysctl_bsd.go
Outdated
| ) | ||
| } | ||
|
|
||
| if len(raw) < (C.sizeof_long) { |
There was a problem hiding this comment.
I'd feel a little better if we checked explicitly to ensure it matches the size of an integer, and then threw an error otherwise.
There was a problem hiding this comment.
Thanks, I feel better about this too. I've had short in the back of my head the whole time. And maybe this thing becomes bsdSysctlTypeSignedInteger, if it's needed.
collector/sysctl_bsd.go
Outdated
| // Not sure this is valid for all CLongs. It is at | ||
| // least for vfs.bufspace: | ||
| // https://github.com/freebsd/freebsd/blob/releng/10.3/sys/kern/vfs_bio.c#L338 | ||
| tmpf64 = float64(*(*C.int)(unsafe.Pointer(&raw[0]))) |
There was a problem hiding this comment.
As it stands now, this will be overwritten by line 134.
collector/sysctl_bsd.go
Outdated
| // least for vfs.bufspace: | ||
| // https://github.com/freebsd/freebsd/blob/releng/10.3/sys/kern/vfs_bio.c#L338 | ||
| tmpf64 = float64(*(*C.int)(unsafe.Pointer(&raw[0]))) | ||
| break |
There was a problem hiding this comment.
The break at 131 is what stops 134 for overwriting - at least that's my intention.
There was a problem hiding this comment.
Apologies, may have misread the diff.
There was a problem hiding this comment.
Still valid. ;)
derekmarcotte
left a comment
derekmarcotte
left a comment
There was a problem hiding this comment.
What about something like this? Seems a little easier to figure out.
mdlayher
left a comment
mdlayher
left a comment
There was a problem hiding this comment.
LGTM after the error message fix. Also left a Go nit about using an early return instead of nested if blocks.
collector/sysctl_bsd.go
Outdated
| return 0, fmt.Errorf( | ||
| "length of bytes received from sysctl (%d) does not match expected bytes (%d)", | ||
| len(raw), | ||
| C.sizeof_long, |
collector/sysctl_bsd.go
Outdated
| return 0, err | ||
| } | ||
|
|
||
| if len(raw) != (C.sizeof_long) { |
There was a problem hiding this comment.
IMO invert this and move the return from line 152 up to reduce a level of indentation.
mdlayher
left a comment
mdlayher
left a comment
There was a problem hiding this comment.
LGTM! Sorry for the brief back-and-forth.
Good cleanup work too. Will leave for someone else more familiar with BSD for +2.
|
Not at all. Happy for the feedback! |
|
Nice, thanks! LGTM |
* Correct buffer_bytes > INT_MAX on BSD/amd64. The sysctl vfs.bufspace returns either an int or a long, depending on the value. Large values of vfs.bufspace will result in error messages like: couldn't get meminfo: cannot allocate memory This will detect the returned data type, and cast appropriately. * Added explicit length checks per feedback. * Flatten Value() to make it easier to read. * Simplify per feedback. * Fix style. * Doc updates.
* netstat: return nothing when /proc/net/snmp6 not found * Fix off by one in Linux interrupts collector (#721) * Fix off by one in Linux interrupts collector * Fix off by one in CPU column handler. * Add test. * Enable interrupts in end-to-end test. * Add and use sysReadFile in hwmon collector (#728) * xfs: expose correct fields, fix metric names * Correct buffer_bytes > INT_MAX on BSD/amd64. (#712) * Correct buffer_bytes > INT_MAX on BSD/amd64. The sysctl vfs.bufspace returns either an int or a long, depending on the value. Large values of vfs.bufspace will result in error messages like: couldn't get meminfo: cannot allocate memory This will detect the returned data type, and cast appropriately. * Added explicit length checks per feedback. * Flatten Value() to make it easier to read. * Simplify per feedback. * Fix style. * Doc updates. * Release v0.15.1 * [BUGFIX] netstat: return nothing when /proc/net/snmp6 not found #718 * [BUGFIX] Fix off by one in Linux interrupts collector #721 * [BUGFIX] Add and use sysReadFile in hwmon collector #728
* Correct buffer_bytes > INT_MAX on BSD/amd64. The sysctl vfs.bufspace returns either an int or a long, depending on the value. Large values of vfs.bufspace will result in error messages like: couldn't get meminfo: cannot allocate memory This will detect the returned data type, and cast appropriately. * Added explicit length checks per feedback. * Flatten Value() to make it easier to read. * Simplify per feedback. * Fix style. * Doc updates.
3 participants
The sysctl vfs.bufspace returns either an int or a long, depending on
the value. Large values of vfs.bufspace will result in error messages
like:
couldn't get meminfo: cannot allocate memory
This will detect the returned data type, and cast appropriately.
This is an updated fix to the one provided in #710.
I suspect that there'll be more re-factoring here, if other uses show up.