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: Strip " and / from snmpwalk_cache_oid() #7063

Merged
merged 3 commits into from Aug 4, 2017

Conversation

Projects
None yet
3 participants
@laf
Member

laf commented Jul 21, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Fixes: #6993

I've not done any other snmp functions yet but they probably should be done to save trim() being used everywhere.

@laf laf added the Bug 🐞 label Jul 21, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 21, 2017

Member

@laf you could also just modify the previous trim.

snmpwalk_group() trims quotes, spaces, and newlines, but not / why is that needed?

Member

murrant commented Jul 21, 2017

@laf you could also just modify the previous trim.

snmpwalk_group() trims quotes, spaces, and newlines, but not / why is that needed?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 21, 2017

Member

@murrant I left the old trim in place as that trims the defaults off, if you specify what to trim that doesn't happen anymore does it? I thought it didn't hence adding the new trim.

The / is in the snmpwalk output so seems to be sent back by the device. I've seen this in other walks as well.

Member

laf commented Jul 21, 2017

@murrant I left the old trim in place as that trims the defaults off, if you specify what to trim that doesn't happen anymore does it? I thought it didn't hence adding the new trim.

The / is in the snmpwalk output so seems to be sent back by the device. I've seen this in other walks as well.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 21, 2017

Member

You can include the defaults.

This is what I have snmpwalk_group(), which is not the full defaults those are " \t\n\r\0\x0B"

trim($value, "\" \n\r")
Member

murrant commented Jul 21, 2017

You can include the defaults.

This is what I have snmpwalk_group(), which is not the full defaults those are " \t\n\r\0\x0B"

trim($value, "\" \n\r")
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 21, 2017

Member

I couldn't find an example of \ in the linked bug. I did however see an example of "\" where this would cause it to return instead of \

Member

murrant commented Jul 21, 2017

I couldn't find an example of \ in the linked bug. I did however see an example of "\" where this would cause it to return instead of \

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 21, 2017

Member

The poller output in the bug report shows what is returned now:

\Core: Raymon[1Gbit](Uplink Raymon) {O000002}\

Seeing as we also have to trim " I've added them both in, I can update and add what you use for snmpwalk_group as well.

Member

laf commented Jul 21, 2017

The poller output in the bug report shows what is returned now:

\Core: Raymon[1Gbit](Uplink Raymon) {O000002}\

Seeing as we also have to trim " I've added them both in, I can update and add what you use for snmpwalk_group as well.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 22, 2017

Member

@laf the " is because the " is in the string input by the user and the snmp daemon escapes it...

Look at this one: \"Cust: Raymon[1Gbit](crs2.dro2.raqxs.net Eth3 - Trunk) {O000002}

We can probably update them all to "\" \\\n\r", but I'm not 100% sure that won't cause issues.

Member

murrant commented Jul 22, 2017

@laf the " is because the " is in the string input by the user and the snmp daemon escapes it...

Look at this one: \"Cust: Raymon[1Gbit](crs2.dro2.raqxs.net Eth3 - Trunk) {O000002}

We can probably update them all to "\" \\\n\r", but I'm not 100% sure that won't cause issues.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 22, 2017

Member

Which of those are you concerned about as it looks like we should strip them all.

Member

laf commented Jul 22, 2017

Which of those are you concerned about as it looks like we should strip them all.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 22, 2017

Member

laf mostly, the \ because it should only be trimmed if it precedes "

It is likely that particular edge case will never cause us any issues.

Member

murrant commented Jul 22, 2017

laf mostly, the \ because it should only be trimmed if it precedes "

It is likely that particular edge case will never cause us any issues.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 22, 2017

Member

It's a bit of a rabbit whole this.

This is what I've come up with so far but I feel less comfortable about this solution then I do my original:

https://p.libren.ms/view/raw/b6afe033

Member

laf commented Jul 22, 2017

It's a bit of a rabbit whole this.

This is what I've come up with so far but I feel less comfortable about this solution then I do my original:

https://p.libren.ms/view/raw/b6afe033

@laf laf added this to the 1.30 milestone Jul 28, 2017

@murrant murrant modified the milestones: 1.31, 1.30 Jul 28, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Aug 2, 2017

Member

Your paste isn't loading. What do you think about just trimming: "\" \\\n\r" and seeing how it goes?

Member

murrant commented Aug 2, 2017

Your paste isn't loading. What do you think about just trimming: "\" \\\n\r" and seeing how it goes?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 2, 2017

Member

Updated

Member

laf commented Aug 2, 2017

Updated

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Aug 3, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Aug 3, 2017

The inspection completed: No new issues

@laf laf merged commit e627472 into librenms:master Aug 4, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.