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 a panic by gnmi subscribe to DB change with invalid xpath #309

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

wumiaont
Copy link
Contributor

Why I did it

It is found that if I run gnmi subscribe to db change with an invalid xpath, gnmi server has a panic because of nil pointer dereference.

Test is against multi-asic chassis. In telemtery test test_on_change_updates is did not handle the multi-asic properly which sends out gnmi subscribe like this: python /root/gnxi/gnmi_cli_py/py_gnmicli.py -g -t 10.250.6.233 -p 50052 -m subscribe -x NEIGH_STATE_TABLE -xt STATE_DB -o ndastreamingservertest --subscribe_mode 0 --submode 1 --interval 0 --update_count 2 --create_connections 1

gnmi container restarts with a panic.

Correct CLI commands should be: python /root/gnxi/gnmi_cli_py/py_gnmicli.py -g -t 10.250.6.233 -p 50052 -m subscribe -x NEIGH_STATE_TABLE/asic0 -xt STATE_DB -o ndastreamingservertest --subscribe_mode 0 --submode 1 --interval 0 --update_count 2 --create_connections 1

How I did it

Fix the deference of nil pointer with this scenario.

How to verify it

With modified code, the above CLI will not cause gnmi panic. The connection is properly closed.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202405

@zbud-msft
Copy link
Contributor

Introduced by #267

@zbud-msft zbud-msft requested a review from ganglyu November 4, 2024 18:15
@arlakshm
Copy link

/Azp run sonic-net.sonic-gnmi

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rlhui rlhui merged commit d77883d into sonic-net:master Nov 22, 2024
5 checks passed
@bingwang-ms
Copy link

@wumiaont Please raise a PR to 202405 branch to address cherry-pick conflict

@wumiaont
Copy link
Contributor Author

wumiaont commented Dec 3, 2024

#267 which introduced this issue was not merged to 202405 so no need to put this fix into 202405. Please remove the "[Request for 202405 Branch]" label

@wumiaont
Copy link
Contributor Author

wumiaont commented Dec 3, 2024

@bingwang-ms @zbud-msft Please help to remove the Request for 202405 Branch label as issues was not in 202405.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants