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

apport-unpack: fix reading from stdin #255

Merged
merged 4 commits into from Dec 8, 2023

Conversation

bdrung
Copy link
Collaborator

@bdrung bdrung commented Nov 30, 2023

Reading a report from stdin fails:

Traceback (most recent call last):
  File "bin/apport-unpack", line 91, in <module>
    main()
  File "bin/apport-unpack", line 70, in main
    report.load(report_file, binary=False)
  File "problem_report.py", line 155, in load
    self._assert_bin_mode(file)
  File "problem_report.py", line 792, in _assert_bin_mode
    assert not hasattr(file, "encoding"), "file stream must be in binary mode"
AssertionError: file stream must be in binary mode

This failure has been found by mypy after adding type hints to ProblemReport.load.

Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
apport-unpack opens the report file two times. Refactor the code to move
the duplicate file opening into `open_report`.

Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
Calling `report.extract_keys` is only needed if there are binary keys
that need extraction.

Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
Reading a report from stdin fails:

```
Traceback (most recent call last):
  File "bin/apport-unpack", line 91, in <module>
    main()
  File "bin/apport-unpack", line 70, in main
    report.load(report_file, binary=False)
  File "problem_report.py", line 155, in load
    self._assert_bin_mode(file)
  File "problem_report.py", line 792, in _assert_bin_mode
    assert not hasattr(file, "encoding"), "file stream must be in binary mode"
AssertionError: file stream must be in binary mode
```

This failure has been found by mypy after adding type hints to
`ProblemReport.load`.

Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7c9c1cd) 83.80% compared to head (648d445) 83.84%.

Files Patch % Lines
bin/apport-unpack 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   83.80%   83.84%   +0.03%     
==========================================
  Files          93       93              
  Lines       18837    18851      +14     
==========================================
+ Hits        15787    15805      +18     
+ Misses       3050     3046       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@schopin-pro schopin-pro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'm really confused, what's the original bug exactly? It lookt to me that the previous code was always giving in sys.stdin, which is a TextIOWrapper so not binary.

@bdrung
Copy link
Collaborator Author

bdrung commented Dec 7, 2023

That the crash with apport 2.27.0:

$ echo "foo" | apport-unpack - /tmp/foo
Traceback (most recent call last):
  File "/usr/bin/apport-unpack", line 92, in <module>
    main()
  File "/usr/bin/apport-unpack", line 68, in main
    report = load_report(args.report)
             ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/bin/apport-unpack", line 41, in load_report
    report.load(sys.stdin, binary=False)
  File "/usr/lib/python3/dist-packages/problem_report.py", line 151, in load
    self._assert_bin_mode(file)
  File "/usr/lib/python3/dist-packages/problem_report.py", line 769, in _assert_bin_mode
    assert not hasattr(file, "encoding"), "file stream must be in binary mode"
AssertionError: file stream must be in binary mode

Copy link
Contributor

@schopin-pro schopin-pro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, there's a wider issue but it's out of scope.

bin/apport-unpack Show resolved Hide resolved
@schopin-pro schopin-pro merged commit 6a555fb into canonical:main Dec 8, 2023
25 checks passed
@bdrung bdrung deleted the unpack-stdin branch December 8, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants