shell script improvements

[aarondill]

Description of what the PR does

This PR makes the shell scripts safer by handling spaces better, as well as quoting arguments, and moving from the legacy `backtick` syntax to the $(command substitution) syntax

Issue which the PR addresses

Fixes #775

Environment in which the PR was developed (OS, IDE, Java version, etc.)

Only POSIX compliant shell scripts are modified, so my environment should not affect the PR. However, in the event it's still desired, here it is:

> echo $SHELL
/bin/bash
> lsb_release -ds
Ubuntu 22.04.2 LTS
> nvim --version
NVIM v0.10.0-dev
...
> java -version
openjdk version "11.0.19" 2023-04-18
OpenJDK Runtime Environment (build 11.0.19+7-post-Ubuntu-0ubuntu122.04.1)
OpenJDK 64-Bit Server VM (build 11.0.19+7-post-Ubuntu-0ubuntu122.04.1, mixed mode, sharing)

Precise steps for testing the PR (i.e., how to demonstrate that it works correctly)

run them, I suppose. I don't believe there was ever a testing mechanism for the shell launch scripts.

[aarondill]

Having reread the CONTRIBUTING.MD, it seems I was supposed to merge into master, rather than develop? I'd be more than happy to rebase this on master if I receive confirmation that I am indeed supposed to do so.

[lane55]

Some process clarification from

Having reread the CONTRIBUTING.MD, it seems I was supposed to merge into master, rather than develop?
The CONTRIBUTING.MD says tht the maintaining team

"the PC^2 maintainers to merge your code into the PC^2 master branch and thus make it part of an upcoming public distribution of PC^2."

At this time you are not part of the maintainers, so any push you might do to master would
have to be reverted.

I'd be more than happy to rebase this on master if I receive confirmation that I am indeed supposed to do so.

Thanks for the offer, but the maintainers will move the work from PR, to develop, to master.

[lane55]

An issue (bug description) should be created for this PR, we use issues to search for changes the PRs related to any fix.

[lane55]

In the to-be-created issue can you describe why the backtick syntac is less secure?

[lane55]

One of the practices we have is for any PR submitter to test all changes - did you test all 14 scripts?

[lane55]

This change is not related to security, and will require additional testing, there are a number
of rewrites of extisting well tested code.

[aarondill]

I'm sorry, I think you misunderstood my usage of the word "safe" in the description. None of these changes should directly affect the security of the scripts. They simply improve edge-case handling and use more modern shell features to ensure higher success rates across a wide variety of machines.

[aarondill]

This specific change from the [ cond -o cond ] syntax to [ cond ] || [ cond ] syntax should make no measurable difference in a wide majority of cases, however, as these scripts run in /bin/sh, which is often provided by different shells (bash in some systems, dash in others such as mine, etc), it is generally advised to stick to POSIX compatible syntax (such as ||) rather than shell extensions which may not be as portable between machines (such as -o)

[aarondill]

From the POSIX definition of test ([])

expression1 -o  expression2
[OB XSI]
True if either expression1 or expression2 is true; otherwise, false. The -o binary primary is left associative.

In which, the [OB XSI] classification is described as:

The functionality described may be removed in a future version of this volume of POSIX.1-2017. Strictly Conforming POSIX Applications and Strictly Conforming XSI Applications shall not use obsolescent features.

As thus, for future compatibility and portability in all POSIX shells (which /bin/sh is supposed to be, regardless of it's implementation), using [ cond ] || [ cond ] is the recommended syntax over the test operator -o.

[johnbrvc]

I am not a fan of changing all the echo commands to printf commands in the scripts for no apparent reason. If there is a compelling reason to do so, please clarify. printf requires more work on the part of the programmer.

[johnbrvc]

The script uses the 'sudo' command on the commands that require root. This script does not necessarily have to run as root, but, as a user than can become root using sudo. I think adding this restriction changes the operation of the script.

[aarondill]

I ran this script on my own machine and it failed due to permissions unless it was run as root (at the echo > ... commands). I can revert this, but perhaps these commands that require higher permissions should be investigated.

A few notable lines: line 74, and line 120

[aarondill]

I am not a fan of changing all the echo commands to printf commands in the scripts for no apparent reason. If there is a compelling reason to do so, please clarify. printf requires more work on the part of the programmer.

Some of these echo commands can remain if desired, however echo has portability concerns (see this stackexchange answer for more details), which printf doesn't have.

Two options here would be:

1. Create a function (perhaps in pc2env) called log (or something similar) which calls printf in the expected way.
2. Use echo as before, but replace any invocations which include variables or backslashes.

In my personal use cases, I include a function such as the following to ease this problem:

log(){ printf '%s\n' "$@"; }

[aarondill]

I am not a fan of changing all the echo commands to printf commands in the scripts for no apparent reason. If there is a compelling reason to do so, please clarify. printf requires more work on the part of the programmer.

According to the POSIX documentation for echo, printf is suggested to resolve this very problem.

APPLICATION USAGE
It is not possible to use echo portably across all POSIX systems unless both -n (as the first argument) and escape sequences are omitted.

The [printf](https://pubs.opengroup.org/onlinepubs/009695399/utilities/printf.html) utility can be used portably to emulate any of the traditional behaviors of the echo utility as follows (assuming that IFS has its standard value or is unset):

The historic System V echo and the requirements on XSI implementations in this volume of IEEE Std 1003.1-2001 are equivalent to:

printf "%b\n" "$*"

The BSD echo is equivalent to:

if [ "X$1" = "X-n" ]
then
    shift
    printf "%s" "$*"
else
    printf "%s\n" "$*"
fi

New applications are encouraged to use [printf](https://pubs.opengroup.org/onlinepubs/009695399/utilities/printf.html) instead of echo.

[johnbrvc]

I am not a fan of changing all the echo commands to printf commands in the scripts for no apparent reason. If there is a compelling reason to do so, please clarify. printf requires more work on the part of the programmer.

According to the POSIX documentation for echo, printf is suggested to resolve this very problem.

APPLICATION USAGE
It is not possible to use echo portably across all POSIX systems unless both -n (as the first argument) and escape sequences are omitted.

The [printf](https://pubs.opengroup.org/onlinepubs/009695399/utilities/printf.html) utility can be used portably to emulate any of the traditional behaviors of the echo utility as follows (assuming that IFS has its standard value or is unset):

The historic System V echo and the requirements on XSI implementations in this volume of IEEE Std 1003.1-2001 are equivalent to:

printf "%b\n" "$*"

The BSD echo is equivalent to:

if [ "X$1" = "X-n" ]
then
    shift
    printf "%s" "$*"
else
    printf "%s\n" "$*"
fi

New applications are encouraged to use [printf](https://pubs.opengroup.org/onlinepubs/009695399/utilities/printf.html) instead of echo.

I'm not convinced changing echo's to printf's is worth the time required to review, test and approve those changes since it really doesn't buy us anything. (IMHO)

[lane55]

This PR is now being considered for removal.

Here are some reasons:
The primary reason is that there is no comment indicating that the author tested the 21 scripts that were changed
No answers/activity by the author for over 2 months

Other reasons
The regression testing required would take a number of days
It would take a lot of effort to exploit the security hole mentioned, so it would be unlikely that the effort to
test and fix would be a good use of the pc2 team's time. So the cost benefit is very low.

[aarondill]

@lane55 Of course I have tested the scripts. I apologize for the lack of a response to your above comment. Following a discussion between 3 different people across both an issue and a PR at the same time regarding different conversations resulted in your comment being missed.

I have been waiting for a response from the maintainers of the repository regarding what you want done next. Our last communication was here where I expressed willingness to help and continue the process, but received no reply regarding the next steps.

If you don't feel it's worth the time or effort to fix these bugs, then I understand that, however I'd request that this not be closed due solely to inactivity.

[lane55]

@aarondill There is done and done done. Clearly you must have some some testing,
did you test each of these scripts?
1 pc2admin
2 pc2aj
3 pc2board
4 pc2ef
5 pc2extract
6 pc2judge
7 pc2nc
8 pc2report
9 pc2reset
10 pc2rui
11 pc2server
12 pc2submit
13 pc2tapi
14 pc2team
15 pc2tools
16 pc2tt
17 pc2ver
18 pc2zip

Provide step by step detailed instructions about how you tested at least one of the scripts to
show that the change you made actually was used/exercised.

For our team to approve the PR two of us are required to test all 18 scripts and approve a code review.
We are evaluateing whether the system would benefit from these changes enough to justify
the time for our team to test all the scripts.

As indicated above in the comment starting with "This PR is now being considered for removal."
there are reasons to close this PR.

[aarondill]

@lane55 I have tested each script included, evidence of which can be found by the bugs found and fixed in these commits:

4c60e48, 3586e60, 883c7bc

The easiest way to test most of these scripts in to move the /bin folder to /bin with spaces and run each one.
Other changes would be difficult to test without finding extreme edge cases involving IFS and extreme abuse of $0, echo, and strings beginning with -.

I understand that it will take time and effort to test all the scripts and I am happy to help, if possible.

I also understand that you may feel that these improvements are not worth your time and effort, and as I stated in the response to your comment about reasons to close this PR, if you do feel it's not worth it to fix these bugs, then I will understand the closer of this PR, however, as stated above, I was simply requesting that this PR is not closed solely due to inactivity while I wait for communication from your team as to testing and further steps.

[aarondill]

Would it potentially help these changes be merged if this PR is split into several PRs?
For example, I could possibly split these into:

• Change `cmd` to $(cmd) (makes no behavior changes, but is more readable/maintainable)
• Change $* to "$@" to avoid double-splitting arguments
  • if echo.sh is just printf '%s\n' $*, then ./echo.sh "1 2 3" 4 results in 1\n2\n3\n4
  • if echo.sh is just printf '%s\n' "$@", then ./echo.sh "1 2 3" 4 results in 1 2 3\n4
• Quote variables unless splitting is intentional
• Other changes?

These would then be smaller PRs, which would be easier to test/review individually with simpler diffs.

Please let me know what your preferred approach is, as this PR is beginning to become rather old (this is the oldest open PR on this repo), and I'd be happy to do what is necessary to get the changes merged if possible

[johnbrvc]

Would it potentially help these changes be merged if this PR is split into several PRs? For example, I could possibly split these into:

• Change cmd to $(cmd) (makes no behavior changes, but is more readable/maintainable)

• Change $* to "$@" to avoid double-splitting arguments

  • if echo.sh is just printf '%s\n' $*, then ./echo.sh "1 2 3" 4 results in 1\n2\n3\n4
  • if echo.sh is just printf '%s\n' "$@", then ./echo.sh "1 2 3" 4 results in 1 2 3\n4

• Quote variables unless splitting is intentional

• Other changes?

These would then be smaller PRs, which would be easier to test/review individually with simpler diffs.

Please let me know what your preferred approach is, as this PR is beginning to become rather old (this is the oldest open PR on this repo), and I'd be happy to do what is necessary to get the changes merged if possible

We are currently implemented changes and features to support the upcoming contests: PacNW, NAC and WF. Low priority items (such as this one) are kind of on the back burner. We will revisit this at some point, but one of the major problems with a PR such as this is the testing required - and that takes a great deal of time and resources. Since two approvals are required, it means two people have to devote time to doing that, and with so many other more pressing needs, well, it's going to be pushed back.

I don't think separating them will make anything easier, in fact, it's more "paperwork" in that we have then create an issue for each, and a PR for each, and that, is more work for everyone on the team.

Thanks for your interest in helping us develop PC2!