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

switch to using --check #4

Merged
merged 3 commits into from
Nov 1, 2022
Merged

switch to using --check #4

merged 3 commits into from
Nov 1, 2022

Conversation

gavinuhma
Copy link
Owner

Implementing the suggestions from https://news.ycombinator.com/item?id=33377273

checksum.sh Outdated
@@ -1,18 +1,17 @@
#!/bin/bash

function checksum() {
local s
s=$(curl -fsSL "$1")
if ! command -v shasum >/dev/null
then
shasum() { sha1sum "$@"; }

Choose a reason for hiding this comment

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

sha1sum doesn't support the -a option you always give below. You just shouldn't pass -a. I only used it when contriving an example (where I had to mock having something to download which has a hash already).

And you can just use a variable for the command (you don't need eval or anything like that to do such things).

local hasher
if command -v shasum >/dev/null ; then
  hasher=shasum
else
  hasher=sha1sum
fi
...
printf %s\\n "$s" | "$hasher" --check --status ...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated. Great suggestion. Thanks!

@@ -1,18 +1,17 @@
#!/bin/bash

function checksum() {
local s
s=$(curl -fsSL "$1")
Copy link

@dundarious dundarious Oct 31, 2022

Choose a reason for hiding this comment

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

Note that shell truncates trailing newlines from variables, so if a script ends with several newlines, it'll only keep the last one. Example exhibiting the problem:

# In another shell, run `python -m http.server --bind localhost 8980`
$ printf %s\\n 1 2 3 "" "" "" > f  # File contents: "1\n2\n3\n\n\n\n"
$ printf %s\\n 1 2 3 > t  # File contents: "1\n2\n3\n"
$ shasum -a 256 f
09bc489de9097269db796e13a5d79c0bdb021a4ba90e1ac1e7f56aecc60b5b7c *f
$ shasum -a 256 t
14c5e74c4b96ccef41cd94db73a9ec3348038ac094feca4fd897cecffa07cdae *t
$ curl -fsSL http://localhost:8980/f | shasum -a 256
09bc489de9097269db796e13a5d79c0bdb021a4ba90e1ac1e7f56aecc60b5b7c *-
$ curl -fsSL http://localhost:8980/t | shasum -a 256
14c5e74c4b96ccef41cd94db73a9ec3348038ac094feca4fd897cecffa07cdae *-
$ t=$(curl -fsSL http://localhost:8980/t)
$ f=$(curl -fsSL http://localhost:8980/f)
$ printf %s\\n "$t" | shasum -a 256
14c5e74c4b96ccef41cd94db73a9ec3348038ac094feca4fd897cecffa07cdae *-
$ printf %s\\n "$f" | shasum -a 256
14c5e74c4b96ccef41cd94db73a9ec3348038ac094feca4fd897cecffa07cdae *-

So printf %s\\n "$f" | shasum -a 256 does not give the same as curl -fsSL http://localhost:8980/f | shasum -a 256, it gives the same result as curl -fsSL http://localhost:8980/t | shasum -a 256 and printf %s\\n "$t" | shasum -a 256.

It's quite likely there exist scripts in the wild that have trailing newlines, so the shell variable capture is probably non-viable.

(I ran this experiment in msys zsh on windows, but ran it based on knowledge of the newline chomping of POSIX sh -- I just double-checked and ran it on bash on linux and it was the same result, only difference being the weird "*" in the shasum output is now another space as expected)

Copy link
Owner Author

Choose a reason for hiding this comment

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

the shell variable capture is probably non-viable.

@dundarious given that, downloading the script to a tmp file may be the best solution

Copy link
Owner Author

Choose a reason for hiding this comment

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

#6

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

Successfully merging this pull request may close these issues.

2 participants