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

cli: segfault when experiment returns nil, err #1816

Closed
bassosimone opened this issue Oct 14, 2021 · 1 comment
Closed

cli: segfault when experiment returns nil, err #1816

bassosimone opened this issue Oct 14, 2021 · 1 comment
Assignees
@bassosimone
Labels
bug Something isn't working correctly priority/high Important issue that needs attention soon

Comments

@bassosimone
Copy link
Contributor

bassosimone commented Oct 14, 2021 •
edited
Loading

Describe the bug

The code for processing measurements in ooni/probe-cli does not assume as valid the case where the experiment returns nil, err, which however can happen now that we have async measurements.

The culprit is here: https://github.com/ooni/probe-cli/blob/master/cmd/ooniprobe/internal/nettests/nettests.go#L167

To Reproduce

We noticed it when testing this diff: ooni/probe-cli@2918cb5

Expected behavior

There are two sides of this issue:

  1. there are experiments like urlgetter that like to return an error even when they should actually not return an error, because what went wrong is part of the normal network activity they're measuring (this is nettests: do not return result _and_ error #1808)

  2. the code that processes measurements assume this is possible

Screenshots

N/A

System information (please complete the following information):

N/A

Additional context

The problem was introduced in ooni/probe-cli#527
@bassosimone bassosimone added bug Something isn't working correctly priority/high Important issue that needs attention soon interrupt labels Oct 14, 2021
@bassosimone bassosimone added this to the Sprint 50 - Amphipoda milestone Oct 14, 2021
@bassosimone bassosimone self-assigned this Oct 14, 2021
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 14, 2021
@bassosimone
fix(ooniprobe): Measure returns EITHER measurement OR nil now
4552b02 
See ooni/probe#1816

This diff addresses the most immediate issue but there is probably
extra work to do, including testing and making sure experiments
do not return an error when they should not.
@bassosimone bassosimone mentioned this issue Oct 14, 2021
Merged
3 tasks
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 14, 2021
@bassosimone
fix(ooniprobe): Measure returns EITHER measurement OR error now (#544)
730b00f 
See ooni/probe#1816

This diff addresses the most immediate issue but there is probably
extra work to do, including testing and making sure experiments
do not return an error when they should not.
@bassosimone bassosimone removed this from the Sprint 50 - Amphipoda milestone Oct 22, 2021
@bassosimone bassosimone added priority/medium Normal priority issue priority/high Important issue that needs attention soon and removed interrupt priority/high Important issue that needs attention soon priority/medium Normal priority issue labels Nov 8, 2021
@bassosimone bassosimone mentioned this issue Nov 26, 2021
Closed
3 tasks
@bassosimone
Copy link
Contributor Author

This issue has already been fixed in ooni/probe-cli#544. So, I am closing it and I am retroactively assigning this issue to the correct sprint (Sprint 52).

ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
@bassosimone
fix(ooniprobe): Measure returns EITHER measurement OR error now (ooni…
3f1bb43 
…#544)

See ooni/probe#1816

This diff addresses the most immediate issue but there is probably
extra work to do, including testing and making sure experiments
do not return an error when they should not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
bug Something isn't working correctly priority/high Important issue that needs attention soon
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bassosimone