Skip to content

Comments

filesystem_freebsd: Fix label values#1728

Merged
discordianfish merged 4 commits intoprometheus:masterfrom
phyber:20200530-fix_filesystem_freebsd_label_values
Jun 3, 2020
Merged

filesystem_freebsd: Fix label values#1728
discordianfish merged 4 commits intoprometheus:masterfrom
phyber:20200530-fix_filesystem_freebsd_label_values

Conversation

@phyber
Copy link
Contributor

@phyber phyber commented May 30, 2020

Fixes #1727

We must know the length of the various filesystem C strings before
turning them from a byte array into a Go string, otherwise our Go
strings could contain null bytes, corrupting the label values.

We must know the length of the various filesystem C strings before
turning them from a byte array into a Go string, otherwise our Go
strings could contain null bytes, corrupting the label values.

Signed-off-by: David O'Rourke <david.orourke@gmail.com>
@discordianfish
Copy link
Member

Hrmmm isn't that something that Go should take care of? Surprised about that behavior.
/cc @SuperQ @pgier

@phyber
Copy link
Contributor Author

phyber commented Jun 1, 2020

I was surprised here as well, but it looks like intended behaviour. Documentation on the string() cast is kind of hard to find (I found the Go docs hard to browse and couldn't find the relevant source), but it looks like it will just take any bytes and dump them out into a string type instead of stopping at null. I guess this makes sense from a speed point of view, but is surprising in a language like Go.

The closest thing describing this behaviour is the spec which simply says: A string value is a (possibly empty) sequence of bytes. It makes no mention of null being disallowed in a string.

I've added a simple program showing the behaviour below.

$ cat main.go && go run main.go | tee output.log | xxd && cat output.log
package main

import (
        "bytes"
        "fmt"
)

func main() {
        b := []byte{65, 66, 67, 0, 68, 0, 69, 0, 70, 0, 71, 0}
        n := bytes.Index(b, []byte{0})

        bad := string(b)
        good := string(b[:n])

        fmt.Println(bad)
        fmt.Println(good)
}

00000000: 4142 4300 4400 4500 4600 4700 0a41 4243  ABC.D.E.F.G..ABC
00000010: 0a                                       .

ABCDEFG # Null bytes hidden in output on terminal
ABC     # Good string after cast was stopped at first null byte

@discordianfish
Copy link
Member

Thanks for elaborating! Yeah okay then this makes sense. Could you move this into a function though?

@phyber
Copy link
Contributor Author

phyber commented Jun 2, 2020

I'll have this simplified and broken out into its own function in a while. helpers.go looks like a good place for such a function to live, and I'll add a few basic tests for it too.

phyber added 2 commits June 2, 2020 12:07
Signed-off-by: David O'Rourke <david.orourke@gmail.com>
Signed-off-by: David O'Rourke <david.orourke@gmail.com>
@phyber
Copy link
Contributor Author

phyber commented Jun 2, 2020

This should be ready for another review now.

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.

Minor nit, otherwise LGTM. Thanks!

Signed-off-by: David O'Rourke <david.orourke@gmail.com>
@phyber phyber force-pushed the 20200530-fix_filesystem_freebsd_label_values branch from 9f22271 to 85ebfa2 Compare June 2, 2020 12:39
@phyber
Copy link
Contributor Author

phyber commented Jun 2, 2020

Copyright year fixed. Sorry for missing that :)

@SuperQ SuperQ requested a review from discordianfish June 2, 2020 13:26
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.

Awesome, thanks!

@discordianfish discordianfish merged commit 217bbdd into prometheus:master Jun 3, 2020
@SuperQ SuperQ mentioned this pull request Jun 15, 2020
@SuperQ SuperQ mentioned this pull request Feb 5, 2021
SuperQ added a commit that referenced this pull request Feb 5, 2021
* Update Build
  - Update CircleCI orb.
  - Update CIrcleCI Machine image.
  - Use golang-builder 1.15.
* Update Go modules.
* Fixup fixtures for XFS bug.

Changes:
* [CHANGE] Improve filter flag names. #1743
* [CHANGE] Add btrfs and powersupplyclass to list of exporters enabled by default #1897
* [FEATURE] Add fibre channel collector #1786
* [FEATURE] Expose cpu bugs and flags as info metrics. #1788
* [FEATURE] Add network_route collector #1811
* [FEATURE] Add zoneinfo collector #1922
* [ENHANCEMENT] Add more InfiniBand counters #1694
* [ENHANCEMENT] Add flag to aggr ipvs metrics to avoid high cardinality metrics #1709
* [ENHANCEMENT] Adding backlog/current queue length to qdisc collector #1732
* [ENHANCEMENT] Include TCP OutRsts in netstat metrics #1733
* [ENHANCEMENT] Add pool size to entropy collector #1753
* [ENHANCEMENT] Remove CGO dependencies for OpenBSD amd64 #1774
* [ENHANCEMENT] bcache: add writeback_rate_debug stats #1658
* [ENHANCEMENT] Add check state for mdadm arrays via node_md_state metric #1810
* [ENHANCEMENT] Expose XFS inode statistics #1870
* [ENHANCEMENT] Expose zfs zpool state #1878
* [ENHANCEMENT] Added an ability to pass collector.supervisord.url via SUPERVISORD_URL environment variable #1947
* [BUGFIX] filesystem_freebsd: Fix label values #1728
* [BUGFIX] Fix various procfs parsing errors #1735
* [BUGFIX] Handle no data from powersupplyclass #1747
* [BUGFIX] udp_queues_linux.go: s/upd/udp/ in two error strings #1769
* [BUGFIX] Fix node_scrape_collector_success behaviour #1816
* [BUGFIX] Fix NodeRAIDDegraded to not use a string rule expressions #1827
* [BUGFIX] fix: node_md_disks state label from fail to failed #1862
* [BUGFIX] Handle EPERM for syscall in timex collector #1938
* [BUGFIX] bcache: fix typo #1943
* [BUGFIX] Fix XFS read/write stats (prometheus/procfs#343)

Signed-off-by: Ben Kochie <superq@gmail.com>
SuperQ added a commit that referenced this pull request Feb 5, 2021
* Update Build
  - Update CircleCI orb.
  - Update CIrcleCI Machine image.
  - Use golang-builder 1.15.
* Update Go modules.
* Fixup fixtures for XFS bug.

NOTE: We have improved some of the flag naming conventions (PR #1743). The old names are
      deprecated and will be removed in 2.0. They will continue to work for backwards
      compatibility.

* [CHANGE] Improve filter flag names #1743
* [CHANGE] Add btrfs and powersupplyclass to list of exporters enabled by default #1897
* [FEATURE] Add fibre channel collector #1786
* [FEATURE] Expose cpu bugs and flags as info metrics. #1788
* [FEATURE] Add network_route collector #1811
* [FEATURE] Add zoneinfo collector #1922
* [ENHANCEMENT] Add more InfiniBand counters #1694
* [ENHANCEMENT] Add flag to aggr ipvs metrics to avoid high cardinality metrics #1709
* [ENHANCEMENT] Adding backlog/current queue length to qdisc collector #1732
* [ENHANCEMENT] Include TCP OutRsts in netstat metrics #1733
* [ENHANCEMENT] Add pool size to entropy collector #1753
* [ENHANCEMENT] Remove CGO dependencies for OpenBSD amd64 #1774
* [ENHANCEMENT] bcache: add writeback_rate_debug stats #1658
* [ENHANCEMENT] Add check state for mdadm arrays via node_md_state metric #1810
* [ENHANCEMENT] Expose XFS inode statistics #1870
* [ENHANCEMENT] Expose zfs zpool state #1878
* [ENHANCEMENT] Added an ability to pass collector.supervisord.url via SUPERVISORD_URL environment variable #1947
* [BUGFIX] filesystem_freebsd: Fix label values #1728
* [BUGFIX] Fix various procfs parsing errors #1735
* [BUGFIX] Handle no data from powersupplyclass #1747
* [BUGFIX] udp_queues_linux.go: change upd to udp in two error strings #1769
* [BUGFIX] Fix node_scrape_collector_success behaviour #1816
* [BUGFIX] Fix NodeRAIDDegraded to not use a string rule expressions #1827
* [BUGFIX] Fix node_md_disks state label from fail to failed #1862
* [BUGFIX] Handle EPERM for syscall in timex collector #1938
* [BUGFIX] bcache: fix typo in a metric name #1943
* [BUGFIX] Fix XFS read/write stats (prometheus/procfs#343)

Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* Update Build
  - Update CircleCI orb.
  - Update CIrcleCI Machine image.
  - Use golang-builder 1.15.
* Update Go modules.
* Fixup fixtures for XFS bug.

NOTE: We have improved some of the flag naming conventions (PR prometheus#1743). The old names are
      deprecated and will be removed in 2.0. They will continue to work for backwards
      compatibility.

* [CHANGE] Improve filter flag names prometheus#1743
* [CHANGE] Add btrfs and powersupplyclass to list of exporters enabled by default prometheus#1897
* [FEATURE] Add fibre channel collector prometheus#1786
* [FEATURE] Expose cpu bugs and flags as info metrics. prometheus#1788
* [FEATURE] Add network_route collector prometheus#1811
* [FEATURE] Add zoneinfo collector prometheus#1922
* [ENHANCEMENT] Add more InfiniBand counters prometheus#1694
* [ENHANCEMENT] Add flag to aggr ipvs metrics to avoid high cardinality metrics prometheus#1709
* [ENHANCEMENT] Adding backlog/current queue length to qdisc collector prometheus#1732
* [ENHANCEMENT] Include TCP OutRsts in netstat metrics prometheus#1733
* [ENHANCEMENT] Add pool size to entropy collector prometheus#1753
* [ENHANCEMENT] Remove CGO dependencies for OpenBSD amd64 prometheus#1774
* [ENHANCEMENT] bcache: add writeback_rate_debug stats prometheus#1658
* [ENHANCEMENT] Add check state for mdadm arrays via node_md_state metric prometheus#1810
* [ENHANCEMENT] Expose XFS inode statistics prometheus#1870
* [ENHANCEMENT] Expose zfs zpool state prometheus#1878
* [ENHANCEMENT] Added an ability to pass collector.supervisord.url via SUPERVISORD_URL environment variable prometheus#1947
* [BUGFIX] filesystem_freebsd: Fix label values prometheus#1728
* [BUGFIX] Fix various procfs parsing errors prometheus#1735
* [BUGFIX] Handle no data from powersupplyclass prometheus#1747
* [BUGFIX] udp_queues_linux.go: change upd to udp in two error strings prometheus#1769
* [BUGFIX] Fix node_scrape_collector_success behaviour prometheus#1816
* [BUGFIX] Fix NodeRAIDDegraded to not use a string rule expressions prometheus#1827
* [BUGFIX] Fix node_md_disks state label from fail to failed prometheus#1862
* [BUGFIX] Handle EPERM for syscall in timex collector prometheus#1938
* [BUGFIX] bcache: fix typo in a metric name prometheus#1943
* [BUGFIX] Fix XFS read/write stats (prometheus/procfs#343)

Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* Update Build
  - Update CircleCI orb.
  - Update CIrcleCI Machine image.
  - Use golang-builder 1.15.
* Update Go modules.
* Fixup fixtures for XFS bug.

NOTE: We have improved some of the flag naming conventions (PR prometheus#1743). The old names are
      deprecated and will be removed in 2.0. They will continue to work for backwards
      compatibility.

* [CHANGE] Improve filter flag names prometheus#1743
* [CHANGE] Add btrfs and powersupplyclass to list of exporters enabled by default prometheus#1897
* [FEATURE] Add fibre channel collector prometheus#1786
* [FEATURE] Expose cpu bugs and flags as info metrics. prometheus#1788
* [FEATURE] Add network_route collector prometheus#1811
* [FEATURE] Add zoneinfo collector prometheus#1922
* [ENHANCEMENT] Add more InfiniBand counters prometheus#1694
* [ENHANCEMENT] Add flag to aggr ipvs metrics to avoid high cardinality metrics prometheus#1709
* [ENHANCEMENT] Adding backlog/current queue length to qdisc collector prometheus#1732
* [ENHANCEMENT] Include TCP OutRsts in netstat metrics prometheus#1733
* [ENHANCEMENT] Add pool size to entropy collector prometheus#1753
* [ENHANCEMENT] Remove CGO dependencies for OpenBSD amd64 prometheus#1774
* [ENHANCEMENT] bcache: add writeback_rate_debug stats prometheus#1658
* [ENHANCEMENT] Add check state for mdadm arrays via node_md_state metric prometheus#1810
* [ENHANCEMENT] Expose XFS inode statistics prometheus#1870
* [ENHANCEMENT] Expose zfs zpool state prometheus#1878
* [ENHANCEMENT] Added an ability to pass collector.supervisord.url via SUPERVISORD_URL environment variable prometheus#1947
* [BUGFIX] filesystem_freebsd: Fix label values prometheus#1728
* [BUGFIX] Fix various procfs parsing errors prometheus#1735
* [BUGFIX] Handle no data from powersupplyclass prometheus#1747
* [BUGFIX] udp_queues_linux.go: change upd to udp in two error strings prometheus#1769
* [BUGFIX] Fix node_scrape_collector_success behaviour prometheus#1816
* [BUGFIX] Fix NodeRAIDDegraded to not use a string rule expressions prometheus#1827
* [BUGFIX] Fix node_md_disks state label from fail to failed prometheus#1862
* [BUGFIX] Handle EPERM for syscall in timex collector prometheus#1938
* [BUGFIX] bcache: fix typo in a metric name prometheus#1943
* [BUGFIX] Fix XFS read/write stats (prometheus/procfs#343)

Signed-off-by: Ben Kochie <superq@gmail.com>
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.

filesystem_freebsd: label values contain null bytes

3 participants