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.

Sign up for GitHub

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

Jump to bottom

carbon.relay.* metrics get corrupted if a file cluster has a too long path (yes really) #159

Closed
rtoma opened this issue Mar 15, 2016 · 4 comments
Closed

carbon.relay.* metrics get corrupted if a file cluster has a too long path (yes really) #159

rtoma opened this issue Mar 15, 2016 · 4 comments

Comments

@rtoma
Copy link
Contributor

rtoma commented Mar 15, 2016

With v1.10 (and v1.8):

Use this config:

cluster upstream forward 127.0.0.1:12003;
cluster blacklisted file /tmp/1234567890123456789012.log;
match * send to upstream;

Please observe the unused file cluster and a silly long name. Note the length of the path.

Start with ./relay -f ./bla.conf -p 2003 -S 5 -H HOST. Check the metrics send to the upstream cluster and find these:

...
carbon.relays.HOST.metricsSent 1 1458056827
carbon.relays.HOST.metricsQueued 0 1458056827
...

Now add a character to the file cluster path name:

cluster upstream forward 127.0.0.1:12003;
cluster blacklisted file /tmp/12345678901234567890123.log;
match * send to upstream;

The file cluster is still unused.

Start the relay with the same parameters. Check the metrics stream and now you'll find these:

...
carbon_relays_HOST_metricsSent 187 1458056783
carbon_relays_HOST_metricsQueued 0 1458056783
...

See the dots are translated into underscores? That's the issue.

Bonus: the metrics stream now also contains monsters like these:

carbon_relays_HOST_destinations._tmp_12345678901234567890123_logcarbon_relays_HOST_\
  destinations._tmp_12345678901234567890123_logcarbon_relays_HOST_destinations._tmp\
  _12345678901234567890123_logcarbon_relays_HOST_destinations._tmp_12345678901234\
  567890123_logcarbon_relays_HOST_destinations._tmp_12345678901234567890123_logcarb\
  on_relays_HOST_destinations._tmp_12345678901234567890123_logcarbon_relays_HOST\
  _destinations._tmp_12345678901234567890123_logcarbon_relays_HOST_destinations.wall\
  Time_us 22 1458056783

Smells like a bufferoverflow or one-off?
@grobian
Copy link
Owner

grobian commented Mar 15, 2016

errr yeah this is stupid.

grobian added a commit that referenced this issue Mar 15, 2016
@grobian
collector: fix corrupted metrics for file clusters
adad7d3 
This commit fixes two aspects of issue #159.  The first strncpy doesn't
overrun the buffer, but doesn't add trailing \0 either.  This caused the
"corrupted" metrics, because garbage was read, so use plain snprintf
instead to avoid that.  Second, 32 bytes (as originally intended for
IPv4 address) is a bit short, and insufficient for IPv6 addresses or
paths, so enlarge the buffer size so most paths will fit.
@grobian grobian closed this as completed Mar 15, 2016
@rtoma
Copy link
Contributor Author

rtoma commented Mar 15, 2016

Thanks for the quick fix!

...
carbon.relays.HOST.destinations._tmp_12345678901234567890123xxxxxxxxxxxxxxxxxxxxxxxxxxxx_log.wallTime_us 8 1458068838
carbon.relays.HOST.metricsSent 32 1458068838
carbon.relays.HOST.metricsQueued 0 1458068838
...

@grobian
Copy link
Owner

grobian commented Mar 16, 2016

Just to be sure, those xxxxx things are what you expect to see right? Given your config the xxxxx-es shouldn't be there...

@rtoma
Copy link
Contributor Author

rtoma commented Mar 16, 2016

Yeah those x's are as expected. Its my quick & dirty attempt to create a looooooong path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@grobian @rtoma