Page MenuHomeFreeBSD

.hooks: assorted bugfixes and efficiency overhaul, replacing external commands (awk) by shell built-ins
Needs ReviewPublic

Authored by mandree on Sun, Jul 19, 12:51 PM.

Details

Reviewers
None
Group Reviewers
portmgr
ports secteam
Summary

Fix some robustness bugs in the scripts,
and reduce the number of external commands run so as to
speed up sweeping updates to multiple ports.

  • BUGFIX: add IFS=$'\n' to hook scripts so they really parse the common git diff --name-only output line-wise irrespective of what's on the lines. (Adds robustness)
  • BUGFIX: check_category_makefile breaks if a $category/Makefile contains a $portname subdir with "++", we put the portname into a grep -E pattern, which grep rejects with the error 'repetition-operator operand invalid'. Example ports that trigger this: audio/timidity++* and several devel/*, math/* and science port (47 in total).

    FIX by adding a sed command to common.sh that adds a backslash before each special character of a grep -E modern regular expression, and using that.
  • BUGFIX: replace echo by printf %s to print data we don't control. The echo man page tells us to use printf instead of echo if the string could contain dashes or backslash esacpes
  • BUGFIX: check_mk_indentations only printed the last component of an impromperly indented file, which is unhelpful. Print the full repository path ($mkfile) instead.
  • ROBUSTNESS: add a new check_aaa_filesnames script that rejects space, tab, quotes, backslashes, and anything that git's diff --name-only will escape (which includes the newline character).
  • SPEEDUP check_category_makefile, check_created_by, check_files: replace the expensive "run awk process" to split items by shell built-ins such as case/in/esac, printf, read -r.
  • FEATURE: pre-commit: support a PRE_COMMIT_KEEP_GOING env variable, similar to make -k, which will continue running further hook parts even after an error. This allows seeing all hook errors at once and fix them all at once, avoiding a fix-one-&-fail-again cycle.

PR: 296903
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=296903

Test Plan

I have tested this on FreeBSD 15.1-RELEASE-p1 amd64.

  1. ensure your hooks are activated, git config get core.hookspath needs to print .hooks, else set it
  2. create a local branch git checkout -b my-disposable-review-branch
  3. then try to "git add" all sorts of breakage we intend to trap, adding files with invalid characters (blanks, quotes, tabs, newlines), broken indentation (for Mk files down deep), change a category/port/Makefile and remove the relevant SUBDIRS += line from the category/Makefile

The pre-commit-hook should reject the commit.

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mandree created this revision.
mandree edited the test plan for this revision. (Show Details)
mandree changed the edit policy from "All Users" to "No One".

Added "PR: line" to the commit message and re-diffed with git format-patch HEAD^ -U9999 so as to provide full context if reviewers want to look at context.

mandree changed the edit policy from "No One" to "All Users".Tue, Jul 21, 12:36 AM

Hi Matthias,

thanks for the cleanup — the input hardening in particular looks sensible.

I'm a bit curious why ports-secteam was added as a reviewer here. These are commit-side hook scripts, which I'd have read as portmgr territory. If secteam was pulled in specifically for the adversarial-input parts (check_aaa_filenames, the ERE escaping, printf %s, IFS=$'\n'), that's understandable — but I don't think that needs a formal secteam sign-off to proceed. Happy to resign ports-secteam from the review if that was the only reason; just let me know if you actually wanted a security look at something specific.

joneum — ports-secteam