Handle closed standard input gracefully by ben-grande · Pull Request #27 · Kicksecure/helper-scripts

ben-grande · 2025-05-13T18:55:10Z

This pull request changes...

Changes

Ignore NoneType stdin.

Mandatory Checklist

Legal agreements accepted. By contributing to this organisation, you acknowledge you have read, understood, and agree to be bound by these these agreements:

Terms of Service, Privacy Policy, Cookie Policy, E-Sign Consent, DMCA, Imprint

Optional Checklist

The following items are optional but might be requested in certain cases.

I have tested it locally
I have reviewed and updated any documentation if relevant
I am providing new code and test(s) for it

Fixes #26

ArrayBolt3 · 2025-05-27T05:04:43Z

This doesn't quite work, as stdin is used in more than just the lines this PR hardens:

[sysmaint ~]% stcat <&-                                       
Traceback (most recent call last):
  File "/usr/bin/stcat", line 12, in <module>
    main()
  File "/usr/lib/python3/dist-packages/stdisplay/stcat.py", line 21, in main
    for untrusted_text in file_input(encoding="ascii", errors="replace"):
  File "/usr/lib/python3.11/fileinput.py", line 251, in __next__
    line = self._readline()
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/fileinput.py", line 371, in _readline
    self._readline = self._file.readline  # hide FileInput._readline
                     ^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'readline'
zsh: exit 1     stcat <&-
[sysmaint ~]% sttee <&-
Traceback (most recent call last):
  File "/usr/bin/sttee", line 12, in <module>
    main()
  File "/usr/lib/python3/dist-packages/stdisplay/sttee.py", line 19, in main
    for untrusted_text in stdin:
TypeError: 'NoneType' object is not iterable
zsh: exit 1     sttee <&-
[sysmaint ~]% stcatn <&-
Traceback (most recent call last):
  File "/usr/bin/stcatn", line 12, in <module>
    main()
  File "/usr/lib/python3/dist-packages/stdisplay/stcatn.py", line 26, in main
    for line in file_input(encoding="ascii", errors="replace"):
  File "/usr/lib/python3.11/fileinput.py", line 251, in __next__
    line = self._readline()
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/fileinput.py", line 371, in _readline
    self._readline = self._file.readline  # hide FileInput._readline
                     ^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'readline'
zsh: exit 1     stcatn <&-
[sysmaint ~]% stsponge <&-
Traceback (most recent call last):
  File "/usr/bin/stsponge", line 12, in <module>
    main()
  File "/usr/lib/python3/dist-packages/stdisplay/stsponge.py", line 22, in main
    for untrusted_text in stdin:
TypeError: 'NoneType' object is not iterable
zsh: exit 1     stsponge <&-

For sttee and stsponge, we can probably get away with simply exiting after touching all files listed on the command line (or we could even just exit and let the caller figure out what to do if the specified files aren't created). stcat and stcatn are a bit trickier because file_input seems to handle a closed, empty stdin poorly (which arguably is a bug in Python itself). For those, we'd need to detect that stdin is None, detect a - argument in sys.argv and remove it before calling file_input, and then exit in the event that len(sys.argv) == 0. Not pretty, but would probably work.

ben-grande · 2025-06-02T13:49:23Z

I gave up on file_input. There are small tests for closed stdin now.

ben-grande · 2025-06-02T14:19:53Z

The failing tests are unrelated to this PR. It mentions stcatn but it is related to unicode-testscript which is possibly failing due to PCRE2 syntax.

ben-grande · 2025-06-02T14:35:06Z

Ready for review @ArrayBolt3

ArrayBolt3

Mostly good, only a few things I noticed that could use changed.

ArrayBolt3 · 2025-06-03T03:07:07Z

usr/bin/-

I think this file was created on accident.

ArrayBolt3 · 2025-06-03T03:22:56Z

usr/lib/python3/dist-packages/stdisplay/stsponge.py

+    if stdin is not None:
+        for untrusted_text in stdin:
+            untrusted_text_list.append(untrusted_text)
+    if stdin is not None and len(argv) == 1:


Do we really want to always take the else branch here if stdin is None? I don't think we need the stdin check here. The tests also seem to pass for me if I remove the stdin check here.

ArrayBolt3 · 2025-06-03T03:36:48Z

run-tests

+stdin_utils=(stcat stcatn sttee stsponge)
+utils=(stprint stecho "${stdin_utils[@]}")
 cd "${git_toplevel}/usr/bin"
 "${black[@]}" -- "${utils[@]}"
 "${pylint[@]}" -- "${utils[@]}"
 for file in "${utils[@]}"; do
  "${mypy[@]}" -- "${file}"
 done
+
+for util in "${stdin_utils[@]}"; do
+  ./"${util}" <&-
+  ./"${util}" - <&-
+done


If either of sttee or stsponge are called as ./"${util}" - <&-, they will create a file named -. When running the tests, that ends up creating usr/bin/-, which is probably how that file got into thie PR. Can we divide stdin_utils a bit more so that we can skip the - argument for those two utils?

ArrayBolt3 · 2025-06-03T21:42:00Z

run-tests

-utils=(stprint stecho stcat stcatn sttee stsponge)
+stdin_utils_to_stdout=(stcat stcatn)
+stdin_utils_to_file=(sttee stsponge)
+utils=(stprint stecho "${stdin_utils[@]}")


Suggested change

utils=(stprint stecho "${stdin_utils[@]}")

utils=(stprint stecho "${stdin_utils_to_stdout[@]}" "${stdin_utils_to_file[@]}")

Other than this change, I think this is ready to merge.

For: Kicksecure#26

ArrayBolt3

Looks good to me! @adrelanos Ready to merge.

ben-grande force-pushed the st-test branch from 129c2fe to e599613 Compare June 2, 2025 13:49

ben-grande force-pushed the st-test branch 4 times, most recently from 0b2b082 to 49993e2 Compare June 2, 2025 14:14

ArrayBolt3 suggested changes Jun 3, 2025

View reviewed changes

ben-grande force-pushed the st-test branch from 49993e2 to 4071653 Compare June 3, 2025 12:10

ArrayBolt3 reviewed Jun 3, 2025

View reviewed changes

Handle closed standard input gracefully

6bc3b5c

For: Kicksecure#26

ben-grande force-pushed the st-test branch from 4071653 to 6bc3b5c Compare June 4, 2025 01:08

ArrayBolt3 approved these changes Jun 4, 2025

View reviewed changes

adrelanos merged commit d4c3045 into Kicksecure:master Jun 5, 2025
0 of 5 checks passed

	utils=(stprint stecho "${stdin_utils[@]}")
	utils=(stprint stecho "${stdin_utils_to_stdout[@]}" "${stdin_utils_to_file[@]}")

Conversation

ben-grande commented May 13, 2025

Changes

Mandatory Checklist

Optional Checklist

Uh oh!

ArrayBolt3 commented May 27, 2025

Uh oh!

ben-grande commented Jun 2, 2025

Uh oh!

ben-grande commented Jun 2, 2025

Uh oh!

ben-grande commented Jun 2, 2025

Uh oh!

ArrayBolt3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArrayBolt3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants