From 4c54fc81f9bb0e6c427982148f8e13e5f40d0690 Mon Sep 17 00:00:00 2001 From: Steve Ramage Date: Sun, 25 Oct 2026 12:31:43 -0700 Subject: [PATCH 1/2] feat: warn about deprecated and suspicious values systemd accepts (#565) Add warnings (not errors) for values systemd still accepts but warns about, so they are no longer silently treated as fine: - StandardOutput=/StandardError= syslog and syslog+console (obsolete, systemd uses journal / journal+console) - the "!!" prefix on Exec*= lines (no longer supported, ignored) - unknown escape sequences in Exec*= arguments (systemd keeps the backslash and logs a warning) - PIDFile= and socket paths under /var/run (systemd rewrites to /run) - KillMode=none (unsafe) - User=nobody and SocketUser=nobody (systemd logs "not safe" at notice level, so this one is a weak warning) - Address=/Peer= without a prefix length, and unmasked AllowedIPs= - deprecated boolean forms of IPMasquerade= and IPv4LL= Each message says what systemd does and what to write instead. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../DeprecatedGrammarValueAnnotator.kt | 42 ---- .../inspections/ValueWarningInspection.kt | 100 +++++++++ .../semanticdata/ValueWarnings.kt | 57 +++++ .../optionvalues/EnumOptionValues.kt | 21 +- .../optionvalues/ExecOptionValue.kt | 86 ++++++-- .../optionvalues/ExecOutputOptionValue.kt | 17 +- .../optionvalues/NetworkAddressOptionValue.kt | 16 +- .../ai/ConfigParseIpMasqueradeOptionValue.kt | 14 +- .../ai/ConfigParsePidFileOptionValue.kt | 14 +- .../ai/ConfigParseSocketListenOptionValues.kt | 29 ++- .../ConfigParseUserGroupCompatOptionValue.kt | 25 ++- ...nfigParseWireguardAllowedIpsOptionValue.kt | 85 ++++++- .../ai/RouteAddressNextHopOptionValues.kt | 18 +- src/main/resources/META-INF/plugin.xml | 5 +- .../inspectionDescriptions/ValueWarning.html | 12 + .../DeprecatedGrammarValueAnnotatorTest.kt | 46 ---- .../inspections/ValueWarningInspectionTest.kt | 208 ++++++++++++++++++ .../optionvalues/grammar/DeprecationsTest.kt | 7 +- 18 files changed, 672 insertions(+), 130 deletions(-) delete mode 100644 src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotator.kt create mode 100644 src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspection.kt create mode 100644 src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/ValueWarnings.kt create mode 100644 src/main/resources/inspectionDescriptions/ValueWarning.html delete mode 100644 src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotatorTest.kt create mode 100644 src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspectionTest.kt diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotator.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotator.kt deleted file mode 100644 index 4667ad40..00000000 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotator.kt +++ /dev/null @@ -1,42 +0,0 @@ -package net.sjrx.intellij.plugins.systemdunitfiles.annotators - -import com.intellij.lang.annotation.AnnotationHolder -import com.intellij.lang.annotation.Annotator -import com.intellij.lang.annotation.HighlightSeverity -import com.intellij.openapi.util.TextRange -import com.intellij.psi.PsiElement -import com.intellij.psi.util.PsiTreeUtil -import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileProperty -import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileSectionType -import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.SemanticDataRepository -import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.fileClass -import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.GrammarOptionValue -import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.deprecatedTokens -import net.sjrx.intellij.plugins.systemdunitfiles.settings.ExperimentalSettings - -/** - * Flags valid-but-deprecated values with a weak warning (#467), behind the experimental flag. - * - * Generic: any grammar can mark choices deprecated (see [GrammarOptionValue]'s terminals). The first - * user is RestrictAddressFamilies=, warning on kernel-removed families like AF_DECnet. - */ -class DeprecatedGrammarValueAnnotator : Annotator { - - override fun annotate(element: PsiElement, holder: AnnotationHolder) { - if (element !is UnitFileProperty) return - if (!ExperimentalSettings.getInstance(element.project).state.useGrammarParseEngine) return - - val section = PsiTreeUtil.getParentOfType(element, UnitFileSectionType::class.java) ?: return - val value = element.valueText ?: return - val base = element.valueNode?.psi?.textRange?.startOffset ?: return - val fileClass = element.containingFile.fileClass() - val validator = SemanticDataRepository.instance.getOptionValidator(fileClass, section.sectionName, element.key) - if (validator !is GrammarOptionValue) return - - for (deprecated in validator.combinator.deprecatedTokens(value)) { - holder.newAnnotation(HighlightSeverity.WEAK_WARNING, deprecated.message) - .range(TextRange(base + deprecated.start, base + deprecated.end)) - .create() - } - } -} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspection.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspection.kt new file mode 100644 index 00000000..ec5c4b24 --- /dev/null +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspection.kt @@ -0,0 +1,100 @@ +package net.sjrx.intellij.plugins.systemdunitfiles.inspections + +import com.intellij.codeInspection.LocalInspectionTool +import com.intellij.codeInspection.ProblemHighlightType +import com.intellij.codeInspection.ProblemsHolder +import com.intellij.openapi.util.TextRange +import com.intellij.psi.PsiElement +import com.intellij.psi.PsiElementVisitor +import com.intellij.psi.util.PsiTreeUtil +import net.sjrx.intellij.plugins.systemdunitfiles.UnitFileLanguage +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFile +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileSectionGroups +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileValueType +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileVisitor +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.SemanticDataRepository +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.fileClass +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.GrammarOptionValue +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.deprecatedTokens + +/** + * Warns about values systemd accepts but complains about (#565): deprecated or obsolete spellings, unsafe settings, + * and forms systemd silently corrects. Invalid values are the InvalidValue inspection's job; this one only speaks up + * when the value is otherwise accepted. + * + * It reports two sources: + * - tokens a grammar marks deprecated ([deprecatedTokens], e.g. `FlexibleLiteralChoiceTerminal.deprecating`); + * - whatever a validator implementing [ValueWarnings] returns. + */ +class ValueWarningInspection : LocalInspectionTool() { + override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean): PsiElementVisitor { + val file = holder.file + return if (file !is UnitFile || !file.getLanguage().isKindOf(UnitFileLanguage.INSTANCE)) { + PsiElementVisitor.EMPTY_VISITOR + } else MyVisitor(holder) + } + + private class MyVisitor(private val holder: ProblemsHolder) : UnitFileVisitor() { + override fun visitPropertyType(property: UnitFilePropertyType) { + val section = PsiTreeUtil.getParentOfType(property, UnitFileSectionGroups::class.java) ?: return + val value = property.valueText ?: return + val validator = SemanticDataRepository.instance + .getOptionValidator(section.containingFile.fileClass(), section.sectionName, property.key) + + val warnings = mutableListOf() + if (validator is GrammarOptionValue) { + validator.combinator.deprecatedTokens(value).mapTo(warnings) { ValueWarning(it.start, it.end, it.message, ProblemHighlightType.GENERIC_ERROR_OR_WARNING) } + } + if (validator is ValueWarnings) { + warnings += validator.valueWarnings(property.key, value) + } + + if (warnings.isEmpty()) return + val valueElement = property.valueNode.psi + val offsets = valueTextOffsets(valueElement, value) + for (warning in warnings) { + // The warning's span is in valueText (lines joined, whitespace trimmed); map it back onto the value element. + val range = if (offsets != null && warning.start < warning.end && warning.end <= value.length) { + TextRange(offsets[warning.start], offsets[warning.end - 1] + 1) + } else { + TextRange(0, valueElement.textLength) + } + holder.registerProblem(valueElement, warning.message, warning.highlightType, range) + } + } + } +} + +/** + * For each character of [value] (the element's valueText), its offset inside [valueElement]. Mirrors + * UnitFileValueType.getValue(): each value line is trimmed (String.trim(), i.e. chars <= ' '), a trailing continuation + * backslash becomes a space, and the joined result is trimmed again. Null if the reconstruction does not match [value]. + */ +private fun valueTextOffsets(valueElement: PsiElement, value: String): IntArray? { + val text = StringBuilder() + val offsets = mutableListOf() + var child = valueElement.firstChild + while (child != null) { + if (UnitFileValueType.valueTypes.contains(child.node.elementType)) { + val line = child.text + var from = 0 + var to = line.length + while (from < to && line[from] <= ' ') from++ + while (to > from && line[to - 1] <= ' ') to-- + for (i in from until to) { + text.append(if (i == to - 1 && line[i] == '\\') ' ' else line[i]) + offsets += child.startOffsetInParent + i + } + } + child = child.nextSibling + } + var from = 0 + var to = text.length + while (from < to && text[from] <= ' ') from++ + while (to > from && text[to - 1] <= ' ') to-- + if (text.substring(from, to) != value) return null + return offsets.subList(from, to).toIntArray() +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/ValueWarnings.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/ValueWarnings.kt new file mode 100644 index 00000000..4613a19c --- /dev/null +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/ValueWarnings.kt @@ -0,0 +1,57 @@ +package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata + +import com.intellij.codeInspection.ProblemHighlightType + +/** + * A part of a value that systemd accepts but warns about (#565): an obsolete spelling it rewrites, a setting it calls + * unsafe, or a form it silently corrects. [start, end) is the span inside the value; [message] says what systemd does + * with it and what to write instead. [highlightType] follows how loudly systemd itself complains: GENERIC_ERROR_OR_WARNING + * for its warnings, WEAK_WARNING for what it only logs as a notice. + */ +data class ValueWarning(val start: Int, val end: Int, val message: String, val highlightType: ProblemHighlightType) + +/** + * Implemented by option validators that know values systemd accepts with a warning. The ValueWarning inspection + * reports what [valueWarnings] returns. + * + * Contract: return warnings only for values the validator otherwise accepts, so a warning never piles on an error. + */ +interface ValueWarnings { + fun valueWarnings(key: String, value: String): List +} + +/** + * patch_var_run(): an absolute path whose first two components are `var` and `run` is rewritten to /run/ with a + * notice. path_startswith() skips empty and "." components, so `//var/./run/x` counts as well. + * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L573-L601 + */ +fun legacyVarRunWarning(key: String, value: String): ValueWarning? { + if (!value.startsWith("/")) return null + val components = value.split('/').filter { it.isNotEmpty() && it != "." } + if (components.size < 2 || components[0] != "var" || components[1] != "run") return null + val replacement = "/run/" + components.drop(2).joinToString("/") + return ValueWarning( + 0, + value.length, + "$key=$value is below the legacy directory /var/run/; systemd rewrites it to $replacement and logs a notice. " + + "Use $replacement instead.", + highlightType = ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + ) +} + +/** + * config_parse_in_addr_prefix() with the "warn" ltype (Address= and Peer= in [Network]/[Address]): an address without + * "/N" is accepted with the full-width prefix and a warning. + * https://github.com/systemd/systemd/blob/0f0682b773/src/shared/conf-parser.c#L2114-L2149 + */ +fun missingPrefixLengthWarning(key: String, value: String): ValueWarning? { + if (value.contains('/')) return null + val prefix = if (value.contains(':')) 128 else 32 + return ValueWarning( + 0, + value.length, + "$key=$value has no prefix length; systemd assumes /$prefix and logs a warning. Write the prefix length explicitly, " + + "e.g. $value/$prefix for a single address or the length of the subnet.", + highlightType = ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + ) +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/EnumOptionValues.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/EnumOptionValues.kt index 9f637104..f182cd8d 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/EnumOptionValues.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/EnumOptionValues.kt @@ -1,8 +1,11 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues import com.google.common.collect.ImmutableSet +import com.intellij.codeInspection.ProblemHighlightType import com.intellij.openapi.project.Project import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.Validator +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BOOLEAN import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.EOF @@ -34,7 +37,23 @@ class EmergencyActionOptionValue : AbstractGrammarEnumOptionValue(validOptions, } } -class KillModeOptionValue : AbstractGrammarEnumOptionValue(validOptions, VALIDATOR_NAME) { +class KillModeOptionValue : AbstractGrammarEnumOptionValue(validOptions, VALIDATOR_NAME), ValueWarnings { + + /** + * KillMode=none is accepted with a warning that it is unsafe and deprecated. + * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L871-L876 + */ + override fun valueWarnings(key: String, value: String): List { + if (value != "none") return emptyList() + return listOf( + ValueWarning( + 0, value.length, + "$key=none is unsafe and deprecated: systemd stops managing the service's processes, so they keep running after " + + "the service is stopped or restarted. Use mixed or control-group instead.", + highlightType = ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + ) + ) + } companion object { private val validOptions: Set = ImmutableSet.of("control-group", "process", "mixed", "none") diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOptionValue.kt index a6114bc7..1bc7cad5 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOptionValue.kt @@ -6,6 +6,8 @@ import com.intellij.openapi.project.Project import com.intellij.openapi.util.TextRange import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.Validator +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar import java.util.regex.Pattern @@ -19,7 +21,7 @@ import java.util.regex.Pattern * otherwise a weak warning recommending an absolute path: * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L882-L1094 */ -class ExecOptionValue : OptionValueInformation { +class ExecOptionValue : OptionValueInformation, ValueWarnings { override fun getAutoCompleteOptions(project: Project): Set { return emptySet() } @@ -63,18 +65,58 @@ class ExecOptionValue : OptionValueInformation { * an argument is an error. A path that uses a known specifier is not checked further, because what it expands to * depends on the unit name and the machine; a path whose only '%'s are literal (`%%`, or '%' before a character * that is not a letter or digit) is checked after expanding them. Warnings that systemd logs while still loading - * the command (unknown escape sequences, the obsolete `!!` prefix) are not reported. + * the command (unknown escape sequences, the obsolete `!!` prefix) are [valueWarnings] instead. */ - override fun getErrorMessage(value: String): String? { + override fun getErrorMessage(value: String): String? = analyze(value).error + + /** + * What systemd warns about while still loading the command line: the `!!` prefix, which it ignores, and escape + * sequences cunescape does not know, which it keeps verbatim. Empty when [getErrorMessage] reports an error. + * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L960-L970 + * https://github.com/systemd/systemd/blob/0f0682b773/src/basic/extract-word.c#L204-L222 + */ + override fun valueWarnings(key: String, value: String): List { + val analysis = analyze(value) + if (analysis.error != null) return emptyList() + val lead = value.length - value.trimStart(' ', '\t', '\n', '\r').length + val warnings = mutableListOf() + for (range in analysis.ignoredPrefixes) { + warnings += ValueWarning( + lead + range.first, lead + range.last + 1, + "The !! prefix is no longer supported; systemd ignores it and runs the command as if it had no ! prefix. Remove !!.", + highlightType = ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + ) + } + for (at in analysis.unknownEscapes) { + val escape = value.substring(lead + at, lead + at + 2) + warnings += ValueWarning( + lead + at, lead + at + 2, + "$escape is not a valid escape sequence; systemd keeps the backslash and logs \"Ignoring unknown escape sequences\". " + + // "\$" is usually meant to stop variable expansion, which "$$" does in an Exec*= line. + (if (escape == "\\$") "Write $$ for a literal $, or \\\\ for a literal backslash." else "Write \\\\ for a literal backslash."), + highlightType = ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + ) + } + return warnings + } + + /** The verdict of one pass over a command line: an error, or the spans systemd warns about. */ + private class Analysis(val error: String?, val ignoredPrefixes: List = emptyList(), val unknownEscapes: List = emptyList()) + + private fun analyze(value: String): Analysis { val rvalue = value.trim(' ', '\t', '\n', '\r') var p: Int? = 0 + val ignoredPrefixes = mutableListOf() + val unknownEscapes = mutableListOf() + fun error(message: String) = Analysis(message) // https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L913-L1091 do { var semicolon = false - val first = extractFirstWordAndWarn(rvalue, p) ?: return UNBALANCED_QUOTING - val firstWord = first.word ?: return null + val firstStart = p?.let { skipWhitespace(rvalue, it) } + val first = extractFirstWordAndWarn(rvalue, p, unknownEscapes) ?: return error(UNBALANCED_QUOTING) + val firstWord = first.word ?: return Analysis(null, ignoredPrefixes, unknownEscapes) p = first.next if (firstWord == ";") { @@ -112,6 +154,9 @@ class ExecOptionValue : OptionValueInformation { } f++ } + if (ambientHack && firstStart != null) { + ignoredPrefixes += firstStart until firstStart + f + } val rawPath = firstWord.substring(f) val unknownInPath = firstUnknownSpecifier(rawPath) // null when the path uses a real specifier @@ -124,20 +169,20 @@ class ExecOptionValue : OptionValueInformation { } else if (unknownInPath != null) { // unit_path_printf() fails with -EBADSLT, // https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L992-L998 - return "Unknown specifier %$unknownInPath in executable path: $rawPath" + return error("Unknown specifier %$unknownInPath in executable path: $rawPath") } else if (path != null) { // https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L992-L1028 if (path.isEmpty()) { - return "Empty path in command line" + return error("Empty path in command line") } if (!stringIsSafe(path)) { - return "Executable path contains special characters (control characters, quotes, backslashes or *?[): $path" + return error("Executable path contains special characters (control characters, quotes, backslashes or *?[): $path") } if (pathImpliesDirectory(path)) { - return "Executable path specifies a directory: $path" + return error("Executable path specifies a directory: $path") } if (!filenameOrAbsolutePathIsValid(path)) { - return "Neither a valid executable name nor an absolute path: $path" + return error("Neither a valid executable name nor an absolute path: $path") } if (!separateArgv0) { argCount++ @@ -165,7 +210,7 @@ class ExecOptionValue : OptionValueInformation { continue } - val arg = extractFirstWordAndWarn(rvalue, p) ?: return UNBALANCED_QUOTING + val arg = extractFirstWordAndWarn(rvalue, p, unknownEscapes) ?: return error(UNBALANCED_QUOTING) if (arg.word == null) { break } @@ -174,18 +219,18 @@ class ExecOptionValue : OptionValueInformation { // https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L1060-L1066 val unknown = firstUnknownSpecifier(arg.word) if (unknown != null) { - return "Unknown specifier %$unknown in argument: ${arg.word}" + return error("Unknown specifier %$unknown in argument: ${arg.word}") } argCount++ } // https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L1072-L1077 if (argCount == 0) { - return "Empty executable name or zeroeth argument: with the '@' prefix the first argument after the path is argv[0]" + return error("Empty executable name or zeroeth argument: with the '@' prefix the first argument after the path is argv[0]") } } while (semicolon || firstWord == ";") - return null + return Analysis(null, ignoredPrefixes, unknownEscapes) } override val validatorName: String @@ -288,19 +333,21 @@ class ExecOptionValue : OptionValueInformation { /** * extract_first_word_and_warn(…, EXTRACT_UNQUOTE|EXTRACT_CUNESCAPE): an invalid escape sequence is only a * warning (systemd retries with EXTRACT_UNESCAPE_RELAX and keeps the backslash), so this returns null only for - * unbalanced quoting. + * unbalanced quoting. When the relaxed retry is what succeeds, the offsets of the backslashes it kept are added to + * [unknownEscapes] (systemd logs "Ignoring unknown escape sequences"). * https://github.com/systemd/systemd/blob/0f0682b773/src/basic/extract-word.c#L190-L233 */ - private fun extractFirstWordAndWarn(s: String, from: Int?): Word? { - return extractFirstWord(s, from, false) ?: extractFirstWord(s, from, true) + private fun extractFirstWordAndWarn(s: String, from: Int?, unknownEscapes: MutableList): Word? { + return extractFirstWord(s, from, false, null) ?: extractFirstWord(s, from, true, unknownEscapes) } /** * extract_first_word() with WHITESPACE separators and EXTRACT_UNQUOTE|EXTRACT_CUNESCAPE (plus - * EXTRACT_UNESCAPE_RELAX when [unescapeRelax]). Returns null for -EINVAL. + * EXTRACT_UNESCAPE_RELAX when [unescapeRelax]). Returns null for -EINVAL. In relaxed mode the offset of each + * backslash kept verbatim is added to [keptBackslashes]. * https://github.com/systemd/systemd/blob/0f0682b773/src/basic/extract-word.c#L10-L188 */ - private fun extractFirstWord(s: String, from: Int?, unescapeRelax: Boolean): Word? { + private fun extractFirstWord(s: String, from: Int?, unescapeRelax: Boolean, keptBackslashes: MutableList?): Word? { var i = from ?: return Word(null, null) while (i < s.length && s[i] in WHITESPACE) { i++ @@ -328,6 +375,7 @@ class ExecOptionValue : OptionValueInformation { i += consumed - 1 } else if (unescapeRelax) { out.append('\\').append(c) + keptBackslashes?.add(i - 1) } else { return null } diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOutputOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOutputOptionValue.kt index b532e214..410a9951 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOutputOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ExecOutputOptionValue.kt @@ -1,14 +1,17 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues +import com.intellij.codeInspection.ProblemHighlightType import com.intellij.openapi.project.Project import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.Validator +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar /** * StandardOutput= / StandardError=, config_parse_exec_output(): * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L1329-L1453 */ -class ExecOutputOptionValue() : OptionValueInformation { +class ExecOutputOptionValue() : OptionValueInformation, ValueWarnings { override fun getAutoCompleteOptions(project: Project): Set { return emptySet() } @@ -61,6 +64,16 @@ class ExecOutputOptionValue() : OptionValueInformation { } } + /** + * syslog and syslog+console are rewritten to journal and journal+console with an "obsolete" notice. + * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L1371-L1377 + * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L1428-L1431 + */ + override fun valueWarnings(key: String, value: String): List { + val replacement = OBSOLETE_OUTPUTS[value] ?: return emptyList() + return listOf(ValueWarning(0, value.length, "$key=$value is obsolete; systemd treats it as $replacement. Use $replacement instead.", ProblemHighlightType.GENERIC_ERROR_OR_WARNING)) + } + override val validatorName: String get() = VALIDATOR_NAME @@ -70,6 +83,8 @@ class ExecOutputOptionValue() : OptionValueInformation { private const val FDNAME_MAX = 255 + private val OBSOLETE_OUTPUTS = mapOf("syslog" to "journal", "syslog+console" to "journal+console") + // An absolute path whose components after any leading "", "." and ".." ones (dropped by path_simplify()) are // never "..". private val ABSOLUTE_NORMALIZED_PATH = """/(?:\.{0,2}(?:/|$))*(?!(?:.*/)?\.\.(?:/|$))(?:[^%]|%%|%(?![A-Za-z0-9%]))*""".toRegex() diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/NetworkAddressOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/NetworkAddressOptionValue.kt index d9b7e26d..431a0e7d 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/NetworkAddressOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/NetworkAddressOptionValue.kt @@ -1,15 +1,20 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.Validator +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.missingPrefixLengthWarning import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.CIDR_SEPARATOR import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.EOF import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.GrammarOptionValue import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.IPV4_ADDR import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.IPV6_ADDR +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ParseOutcome import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.SafeAtoTerminal import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.SequenceCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ZeroOrOne +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.validate /* * [Address] Address= (and the [Network] Address= shorthand) in a .network file. @@ -31,7 +36,8 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * > Please specify the prefix length explicitly. * * That is advice, not a rejection: the address is used with the full-width prefix. So a bare address is - * accepted for both families (`Address=2001:db8::1` used to be flagged for IPv6; issue #568). + * accepted for both families (`Address=2001:db8::1` used to be flagged for IPv6; issue #568), and the + * ValueWarning inspection repeats systemd's warning (#565). * * The prefix length goes through in_addr_parse_prefixlen → safe_atou8 (base 0, so `/0x40` works, and * whitespace after the slash reaches it: `/ 8`), and is refused only when wider than the address, so the @@ -40,7 +46,13 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * * `Peer=` is the same ConfigSectionParser slot; see ConfigParseAddressSectionPeerOptionValue. */ -class NetworkAddressOptionValue() : GrammarOptionValue("config_parse_address_section", GRAMMAR) { +class NetworkAddressOptionValue() : GrammarOptionValue("config_parse_address_section", GRAMMAR), ValueWarnings { + + /** A bare address gets the full-width prefix and a warning (see the class comment). */ + override fun valueWarnings(key: String, value: String): List { + if (combinator.validate(value) != ParseOutcome.Valid) return emptyList() + return listOfNotNull(missingPrefixLengthWarning(key, value)) + } companion object { val GRAMMAR = SequenceCombinator( diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseIpMasqueradeOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseIpMasqueradeOptionValue.kt index ba26b18b..d7dd7911 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseIpMasqueradeOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseIpMasqueradeOptionValue.kt @@ -22,16 +22,12 @@ class ConfigParseIpMasqueradeOptionValue : SimpleGrammarOptionValues( ) ) { private companion object { - // systemd.network(5): the boolean forms "are now deprecated. Please use one of the values - // above" (ipv4/ipv6/both/no). Still accepted; the truthy forms map to ipv4. + // Only a true boolean draws a warning: systemd maps it to ipv4 (not both) and says it is deprecated. A false + // boolean silently means "no", which is also a table value, so it is not flagged. + // https://github.com/systemd/systemd/blob/0f0682b773/src/network/networkd-util.c#L176-L185 val DEPRECATED_BOOLEAN = FlexibleLiteralChoiceTerminal(*BOOLEAN_SPELLINGS, ignoreCase = true).deprecating( - buildMap { - for (t in listOf("1", "yes", "y", "true", "t", "on")) { - put(t, "Boolean values for IPMasquerade= are deprecated; use \"ipv4\" (or \"ipv6\"/\"both\") instead.") - } - for (f in listOf("0", "n", "false", "f", "off")) { - put(f, "Boolean values for IPMasquerade= are deprecated; use \"no\" instead.") - } + listOf("1", "yes", "y", "true", "t", "on").associateWith { + "IPMasquerade= with a true boolean is deprecated; systemd treats it as ipv4, not both. Use ipv4 or both." } ) } diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParsePidFileOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParsePidFileOptionValue.kt index 574dc291..cb68b8b3 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParsePidFileOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParsePidFileOptionValue.kt @@ -1,12 +1,17 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.legacyVarRunWarning import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar.NOT_API_VFS import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar.NO_DOT_DOT_COMPONENT import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar.UNIT_TEXT_CHAR import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.EOF +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ParseOutcome import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.RegexTerminal import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.SequenceCombinator +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.validate /* * [Service] PIDFile= @@ -37,4 +42,11 @@ class ConfigParsePidFileOptionValue : SimpleGrammarOptionValues( RegexTerminal("""[\s\S]+""", """$NO_DOT_DOT_COMPONENT$NOT_API_VFS$UNIT_TEXT_CHAR+$"""), EOF(), ) -) +), ValueWarnings { + + /** A path below /var/run/ is rewritten to /run/ by patch_var_run() with a notice. */ + override fun valueWarnings(key: String, value: String): List { + if (combinator.validate(value) != ParseOutcome.Valid) return emptyList() + return listOfNotNull(legacyVarRunWarning(key, value)) + } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSocketListenOptionValues.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSocketListenOptionValues.kt index ddd372c6..5f769be5 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSocketListenOptionValues.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSocketListenOptionValues.kt @@ -2,6 +2,9 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai import com.intellij.codeInspection.ProblemsHolder import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.legacyVarRunWarning import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ALTERNATIVE_INTERFACE_NAME import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator @@ -12,6 +15,7 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.IPV4_ADDR import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.IPV6_ADDR import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.LiteralChoiceTerminal +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ParseOutcome import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.RegexTerminal import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.SafeAtoTerminal import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.SequenceCombinator @@ -20,6 +24,7 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar.UNIT_SPECIFIER_LETTERS import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar.UNIT_TEXT_CHAR import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ZeroOrOne +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.validate import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BatchUnitCoreGrammar.withUnitSpecifiers /* @@ -210,7 +215,7 @@ private val NETLINK_ADDRESS = withUnitSpecifiers( class ConfigParseSocketListenSocketOptionValue : SimpleGrammarOptionValues( "config_parse_socket_listen", AlternativeCombinator(SOCKET_ADDRESS, NETLINK_ADDRESS) -) { +), ValueWarnings { private val byKey: Map = mapOf( "ListenStream" to SimpleGrammarOptionValues("config_parse_socket_listen", SOCKET_ADDRESS), "ListenDatagram" to SimpleGrammarOptionValues("config_parse_socket_listen", SOCKET_ADDRESS), @@ -225,6 +230,16 @@ class ConfigParseSocketListenSocketOptionValue : SimpleGrammarOptionValues( val delegate = byKey[property.key] if (delegate != null) delegate.generateProblemDescriptors(property, holder) else super.generateProblemDescriptors(property, holder) } + + /** + * ListenStream=, ListenDatagram= and ListenSequentialPacket= run patch_var_run() on an absolute (AF_UNIX) path; + * ListenNetlink= never takes a path. + * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L690-L694 + */ + override fun valueWarnings(key: String, value: String): List { + if (key == "ListenNetlink" || combinatorForKey(key).validate(value) != ParseOutcome.Valid) return emptyList() + return listOfNotNull(legacyVarRunWarning(key, value)) + } } /** @@ -239,7 +254,17 @@ class ConfigParseSocketListenSocketOptionValue : SimpleGrammarOptionValues( class ConfigParseSocketListenFileOptionValue : SimpleGrammarOptionValues( "config_parse_socket_listen", SequenceCombinator(UNIT_ABSOLUTE_NON_API_VFS_PATH, EOF()) -) +), ValueWarnings { + + /** + * Only ListenFIFO= (SOCKET_FIFO) runs patch_var_run(); ListenMessageQueue= and ListenUSBFunction= keep /var/run/. + * https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L655-L659 + */ + override fun valueWarnings(key: String, value: String): List { + if (key != "ListenFIFO" || combinator.validate(value) != ParseOutcome.Valid) return emptyList() + return listOfNotNull(legacyVarRunWarning(key, value)) + } +} /** * Validator for `[Socket] ListenSpecial=` (ltype SOCKET_SPECIAL): as diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUserGroupCompatOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUserGroupCompatOptionValue.kt index cf17d6c8..d661d18a 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUserGroupCompatOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseUserGroupCompatOptionValue.kt @@ -1,5 +1,8 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import com.intellij.codeInspection.ProblemHighlightType +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.* @@ -43,4 +46,24 @@ class ConfigParseUserGroupCompatOptionValue : SimpleGrammarOptionValues( ), EOF() ) -) +), + ValueWarnings { + + /** + * For a key whose name contains "User" (User=, SocketUser=) the literal name "nobody" is accepted with the notice + * "Special user nobody configured, this is not safe!". Group=nogroup, Group=nobody and the numeric 65534 pass + * silently. https://github.com/systemd/systemd/blob/0f0682b773/src/core/load-fragment.c#L2587-L2594 + */ + override fun valueWarnings(key: String, value: String): List { + if (!key.contains("User") || value != "nobody") return emptyList() + return listOf( + ValueWarning( + 0, value.length, + "$key=nobody is not safe: the service shares every file and process owned by nobody, including files " + + "whose owner cannot be mapped on NFS or in user namespaces. Use DynamicUser=yes or a dedicated system " + + "user instead.", + highlightType = ProblemHighlightType.WEAK_WARNING, + ) + ) + } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseWireguardAllowedIpsOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseWireguardAllowedIpsOptionValue.kt index eb33a27d..ec54f1aa 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseWireguardAllowedIpsOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseWireguardAllowedIpsOptionValue.kt @@ -1,12 +1,22 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import com.intellij.codeInspection.ProblemHighlightType +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.EOF import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.IP_ADDR_AND_SAFE_PREFIX +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ParseOutcome import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.RegexTerminal import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.SequenceCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ZeroOrMore import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ZeroOrOne +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.canonicalizeIpv6 +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.validate +import java.math.BigInteger +import java.net.Inet4Address +import java.net.Inet6Address +import java.net.InetAddress /* * [WireGuardPeer] AllowedIPs= in a .netdev file. @@ -41,4 +51,77 @@ class ConfigParseWireguardAllowedIpsOptionValue : SimpleGrammarOptionValues( ZeroOrOne(ALLOWED_IPS_SEPARATOR), EOF() ) -) +), ValueWarnings { + + /** + * An entry whose address has bits set beyond the prefix length is masked with the warning "Specified address '…' is + * not properly masked, assuming '…'". + * https://github.com/systemd/systemd/blob/0f0682b773/src/network/netdev/wireguard.c#L726-L732 + */ + override fun valueWarnings(key: String, value: String): List { + if (combinator.validate(value) != ParseOutcome.Valid) return emptyList() + return ALLOWED_IPS_ENTRY.findAll(value).mapNotNull { entry -> + val masked = maskedPrefix(entry.value) ?: return@mapNotNull null + ValueWarning( + entry.range.first, entry.range.last + 1, + "$key entry ${entry.value} has host bits set beyond the prefix length; systemd uses $masked instead. " + + "Write $masked.", + highlightType = ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + ) + }.toList() + } +} + +private val ALLOWED_IPS_ENTRY = Regex("[^\\s,]+") + +/** `ADDRESS/N` with the host bits cleared, or null if [entry] has no prefix length, is already masked, or can't be read. */ +private fun maskedPrefix(entry: String): String? { + val slash = entry.indexOf('/') + if (slash < 0) return null + val address = entry.substring(0, slash) + val prefixLength = parseSafeAtou8(entry.substring(slash + 1)) ?: return null + val bytes = if (address.contains(':')) { + // An IPv4-mapped literal (::ffff:a.b.c.d) comes back as an Inet4Address; rebuild its 16 bytes. + when (val parsed = InetAddress.getByName(address)) { + is Inet6Address -> parsed.address + is Inet4Address -> ByteArray(10) + byteArrayOf(-1, -1) + parsed.address + else -> return null + } + } else { + val octets = address.split('.').map { it.toIntOrNull() ?: return null } + if (octets.size != 4 || octets.any { it !in 0..255 }) return null + octets.map { it.toByte() }.toByteArray() + } + val width = bytes.size * 8 + if (prefixLength > width) return null + val value = BigInteger(1, bytes) + val hostBits = BigInteger.ONE.shiftLeft(width - prefixLength).subtract(BigInteger.ONE) + val masked = value.andNot(hostBits) + if (masked == value) return null + // BigInteger.toByteArray() drops leading zero bytes (and may add a sign byte); pad back to the address size. + val raw = ByteArray(bytes.size) { i -> masked.shiftRight(8 * (bytes.size - 1 - i)).toInt().toByte() } + val text = if (bytes.size == 4) { + raw.joinToString(".") { (it.toInt() and 0xff).toString() } + } else if ((0 until 10).all { raw[it] == 0.toByte() } && raw[10] == (-1).toByte() && raw[11] == (-1).toByte()) { + // inet_ntop() prints IPv4-mapped addresses with a dotted tail, as systemd's warning does. + "::ffff:" + (12 until 16).joinToString(".") { (raw[it].toInt() and 0xff).toString() } + } else { + val groups = (0 until 8).joinToString(":") { i -> + Integer.toHexString(((raw[2 * i].toInt() and 0xff) shl 8) or (raw[2 * i + 1].toInt() and 0xff)) + } + canonicalizeIpv6(groups) ?: groups + } + return "$text/$prefixLength" +} + +/** safe_atou8() with base 0 as in_addr_parse_prefixlen() calls it: decimal, 0x hex or leading-0 octal. */ +private fun parseSafeAtou8(text: String): Int? { + val digits = text.removePrefix("+") + val parsed = when { + digits.startsWith("-") -> if (digits.drop(1).all { it == '0' } && digits.length > 1) 0 else null + digits.startsWith("0x") || digits.startsWith("0X") -> digits.drop(2).toIntOrNull(16) + digits.length > 1 && digits.startsWith("0") -> digits.drop(1).toIntOrNull(8) + else -> digits.toIntOrNull() + } + return parsed?.takeIf { it in 0..255 } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt index 1da3f729..ab17f9bb 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/RouteAddressNextHopOptionValues.kt @@ -1,6 +1,11 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarning +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.ValueWarnings +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.missingPrefixLengthWarning import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ParseOutcome +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.validate import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BOOLEAN import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.BOOLEAN_FALSE @@ -95,7 +100,18 @@ class ConfigParseRouteScopeOptionValue : SimpleGrammarOptionValues( */ class ConfigParseAddressSectionPeerOptionValue : SimpleGrammarOptionValues( ADDRESS, SequenceCombinator(IP_ADDR_AND_ANY_PREFIX, EOF()) -) +), ValueWarnings { + + /** + * Peer= goes through the same config_parse_address() as Address=, so a bare peer address gets the full-width + * prefix and a warning. + * https://github.com/systemd/systemd/blob/0f0682b773/src/network/networkd-address.c#L2159 + */ + override fun valueWarnings(key: String, value: String): List { + if (combinator.validate(value) != ParseOutcome.Valid) return emptyList() + return listOfNotNull(missingPrefixLengthWarning(key, value)) + } +} /** * `[Address] Broadcast=` — table entry config_parse_broadcast: a boolean (asking systemd to derive diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml index d4902b93..594c859a 100644 --- a/src/main/resources/META-INF/plugin.xml +++ b/src/main/resources/META-INF/plugin.xml @@ -60,7 +60,6 @@ - + + +

+ Detects values that systemd accepts but warns about when it loads the unit. +

+ +

Examples are obsolete spellings that systemd rewrites (StandardOutput=syslog, paths below +/var/run/), settings systemd calls unsafe (KillMode=none, User=nobody), and values it +silently corrects (an address without a prefix length, a WireGuard AllowedIPs= entry with host bits set). +Each message says what systemd does with the value and what to write instead.

+ + diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotatorTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotatorTest.kt deleted file mode 100644 index cef31a83..00000000 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/annotators/DeprecatedGrammarValueAnnotatorTest.kt +++ /dev/null @@ -1,46 +0,0 @@ -package net.sjrx.intellij.plugins.systemdunitfiles.annotators - -import com.intellij.codeInsight.daemon.impl.HighlightInfo -import com.intellij.lang.annotation.HighlightSeverity -import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest -import net.sjrx.intellij.plugins.systemdunitfiles.settings.ExperimentalSettings -import org.junit.Test - -/** End-to-end: a kernel-removed address family gets a weak warning (behind the flag). */ -class DeprecatedGrammarValueAnnotatorTest : AbstractUnitFileTest() { - - override fun tearDown() { - try { - ExperimentalSettings.getInstance(project).state.useGrammarParseEngine = false - } finally { - super.tearDown() - } - } - - private fun enableNewEngine() { - ExperimentalSettings.getInstance(project).state.useGrammarParseEngine = true - } - - private fun weakWarned(highlights: List, text: String) = - highlights.any { it.severity == HighlightSeverity.WEAK_WARNING && it.text == text } - - @Test - fun testRemovedFamilyIsWeakWarned() { - enableNewEngine() - setupFileInEditor("file.service", "[Service]\nRestrictAddressFamilies=AF_INET AF_DECnet") - assertTrue(weakWarned(myFixture.doHighlighting(), "AF_DECnet")) - } - - @Test - fun testCurrentFamilyIsNotWarned() { - enableNewEngine() - setupFileInEditor("file.service", "[Service]\nRestrictAddressFamilies=AF_INET") - assertFalse(weakWarned(myFixture.doHighlighting(), "AF_INET")) - } - - @Test - fun testNoWarningWhenFlagOff() { - setupFileInEditor("file.service", "[Service]\nRestrictAddressFamilies=AF_DECnet") - assertFalse(weakWarned(myFixture.doHighlighting(), "AF_DECnet")) - } -} diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspectionTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspectionTest.kt new file mode 100644 index 00000000..76897cae --- /dev/null +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ValueWarningInspectionTest.kt @@ -0,0 +1,208 @@ +package net.sjrx.intellij.plugins.systemdunitfiles.inspections + +import com.intellij.lang.annotation.HighlightSeverity +import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest +import net.sjrx.intellij.plugins.systemdunitfiles.settings.ExperimentalSettings + +/** + * Values systemd accepts but warns about (#565). Each warned case was confirmed with steve-local/systemd-tools sd-check + * at 0f0682b773; the C reference is on the validator. + */ +class ValueWarningInspectionTest : AbstractUnitFileTest() { + + /** (highlighted text, message) of every ValueWarning highlight; also asserts they are all WARNING severity. */ + private fun warnings(fileName: String, contents: String): List> { + setupFileInEditor(fileName, contents) + enableInspection(ValueWarningInspection::class.java) + val highlights = myFixture.doHighlighting().filter { it.inspectionToolId == "ValueWarning" } + for (h in highlights) { + assertEquals(h.description, HighlightSeverity.WARNING, h.severity) + } + return highlights.map { it.text to it.description } + } + + private fun assertSilent(fileName: String, section: String, vararg lines: String) { + for (line in lines) { + assertEquals(line, emptyList>(), warnings(fileName, "[$section]\n$line")) + } + } + + private fun single(fileName: String, contents: String): Pair = warnings(fileName, contents).single() + + /** + * The single ValueWarning highlight, asserted to be WEAK_WARNING: for what systemd only logs as a notice, not a + * warning (e.g. User=nobody). + */ + private fun singleWeak(fileName: String, contents: String): Pair { + setupFileInEditor(fileName, contents) + enableInspection(ValueWarningInspection::class.java) + val highlight = myFixture.doHighlighting().single { it.inspectionToolId == "ValueWarning" } + assertEquals(highlight.description, HighlightSeverity.WEAK_WARNING, highlight.severity) + return highlight.text to highlight.description + } + + fun testObsoleteStandardOutput() { + assertEquals( + "syslog" to "StandardOutput=syslog is obsolete; systemd treats it as journal. Use journal instead.", + single("a.service", "[Service]\nStandardOutput=syslog") + ) + assertEquals( + "syslog+console" to "StandardError=syslog+console is obsolete; systemd treats it as journal+console. Use journal+console instead.", + single("a.service", "[Service]\nStandardError=syslog+console") + ) + assertSilent( + "a.service", "Service", + "StandardOutput=journal", "StandardOutput=journal+console", "StandardError=kmsg", "StandardOutput=file:/var/log/x", + "StandardInput=null", "StandardOutput=Syslog", + ) + } + + fun testDoubleBangPrefix() { + val (text, message) = single("a.service", "[Service]\nExecStart=!!/bin/true") + assertEquals("!!", text) + assertEquals("The !! prefix is no longer supported; systemd ignores it and runs the command as if it had no ! prefix. Remove !!.", message) + assertEquals("-!!", single("a.service", "[Service]\nExecStartPre=-!!/bin/true").first) + // "+!!" is refused outright (the path becomes "!/bin/true"); that is InvalidValue's error, not a warning here. + assertSilent("a.service", "Service", "ExecStart=!/bin/true", "ExecStart=+/bin/true", "ExecStart=-@/bin/true true", "ExecStart=+!!/bin/true") + } + + fun testUnknownEscapeSequences() { + val (text, message) = single("a.service", "[Service]\nExecStart=/bin/echo \\q") + assertEquals("\\q", text) + assertEquals( + "\\q is not a valid escape sequence; systemd keeps the backslash and logs \"Ignoring unknown escape sequences\". " + + "Write \\\\ for a literal backslash.", + message + ) + val dollar = single("a.service", "[Service]\nExecStart=/bin/sh -c \"echo \\\$x\"") + assertEquals("\\$", dollar.first) + assertTrue(dollar.second, dollar.second.endsWith("Write \$\$ for a literal \$, or \\\\ for a literal backslash.")) + // \x00 is refused by cunescape (NUL), so "\x" is kept verbatim. + assertEquals("\\x", single("a.service", "[Service]\nExecStart=/bin/echo \\x0").first) + assertEquals("\\8", single("a.service", "[Service]\nExecStart=/bin/echo \\8").first) + assertEquals(listOf("\\d", "\\w"), warnings("a.service", "[Service]\nExecStart=/bin/grep -E \"\\d+\\w\"").map { it.first }) + assertSilent( + "a.service", "Service", + "ExecStart=/bin/echo \\n \\t \\\\ \\\" \\' \\a \\b \\f \\r \\v \\s", + "ExecStart=/bin/echo \\x41 \\101 \\u00e9 \\U0001F600", + "ExecStart=/bin/echo a \\;", + "ExecStart=/bin/sh -c 'echo \$\$HOME'", + ) + } + + fun testLegacyVarRun() { + assertEquals( + "/var/run/foo.pid" to "PIDFile=/var/run/foo.pid is below the legacy directory /var/run/; systemd rewrites it to " + + "/run/foo.pid and logs a notice. Use /run/foo.pid instead.", + single("a.service", "[Service]\nPIDFile=/var/run/foo.pid") + ) + assertTrue(single("a.service", "[Service]\nPIDFile=//var/./run/foo.pid").second.contains("rewrites it to /run/foo.pid")) + assertSilent("a.service", "Service", "PIDFile=/run/foo.pid", "PIDFile=foo.pid", "PIDFile=/var/runner/x.pid", "PIDFile=/var/lib/run/x.pid") + + assertTrue(single("a.socket", "[Socket]\nListenStream=/var/run/foo.sock").second.startsWith("ListenStream=/var/run/foo.sock is below")) + assertTrue(single("a.socket", "[Socket]\nListenDatagram=/var/run/foo.sock").second.contains("/run/foo.sock")) + assertTrue(single("a.socket", "[Socket]\nListenFIFO=/var/run/foo.fifo").second.contains("/run/foo.fifo")) + // Only ListenFIFO= and the AF_UNIX socket paths go through patch_var_run(). + assertSilent( + "a.socket", "Socket", + "ListenStream=/run/foo.sock", "ListenStream=80", "ListenStream=@var/run/x", "ListenMessageQueue=/var/run/foo", + "ListenSpecial=/var/run/foo", + ) + } + + fun testKillModeNone() { + assertEquals( + "none" to "KillMode=none is unsafe and deprecated: systemd stops managing the service's processes, so they keep " + + "running after the service is stopped or restarted. Use mixed or control-group instead.", + single("a.service", "[Service]\nKillMode=none") + ) + assertSilent("a.service", "Service", "KillMode=mixed", "KillMode=control-group", "KillMode=process") + } + + fun testUserNobody() { + // A weak warning: systemd only logs this at notice level (LOG_NOTICE, load-fragment.c#L2587-L2594), and the + // service runs normally. + val (text, message) = singleWeak("a.service", "[Service]\nUser=nobody") + assertEquals("nobody", text) + assertTrue(message, message.startsWith("User=nobody is not safe:")) + assertTrue(message, message.endsWith("Use DynamicUser=yes or a dedicated system user instead.")) + assertTrue(singleWeak("a.socket", "[Socket]\nSocketUser=nobody").second.startsWith("SocketUser=nobody is not safe:")) + // systemd only checks keys containing "User" and only the literal name. + assertSilent("a.service", "Service", "Group=nogroup", "Group=nobody", "User=65534", "User=nobody2", "User=daemon") + assertSilent("a.socket", "Socket", "SocketGroup=nobody") + } + + fun testAddressWithoutPrefixLength() { + assertEquals( + "10.0.0.1" to "Address=10.0.0.1 has no prefix length; systemd assumes /32 and logs a warning. Write the prefix " + + "length explicitly, e.g. 10.0.0.1/32 for a single address or the length of the subnet.", + single("a.network", "[Network]\nAddress=10.0.0.1") + ) + assertTrue(single("a.network", "[Address]\nAddress=2001:db8::1").second.contains("assumes /128")) + assertTrue(single("a.network", "[Address]\nPeer=10.0.0.2").second.startsWith("Peer=10.0.0.2 has no prefix length")) + assertSilent("a.network", "Network", "Address=10.0.0.1/24", "Address=2001:db8::1/64", "Address=0.0.0.0/0") + assertSilent("a.network", "Address", "Address=10.0.0.1/24", "Peer=10.0.0.2/32") + } + + fun testWireGuardAllowedIpsNotMasked() { + val found = warnings( + "a.netdev", + "[WireGuardPeer]\nAllowedIPs=192.168.26.3/24,10.0.0.0/8 fd31::1/48" + ) + assertEquals( + listOf( + "192.168.26.3/24" to "AllowedIPs entry 192.168.26.3/24 has host bits set beyond the prefix length; systemd uses " + + "192.168.26.0/24 instead. Write 192.168.26.0/24.", + "fd31::1/48" to "AllowedIPs entry fd31::1/48 has host bits set beyond the prefix length; systemd uses fd31::/48 " + + "instead. Write fd31::/48.", + ), + found + ) + assertTrue(single("a.netdev", "[WireGuardPeer]\nAllowedIPs=10.1.2.0/0x10").second.contains("systemd uses 10.1.0.0/16")) + assertSilent( + "a.netdev", "WireGuardPeer", + "AllowedIPs=10.0.0.0/8", "AllowedIPs=192.168.1.1", "AllowedIPs=0.0.0.0/0, ::/0", "AllowedIPs=fd31:bf08:57cb::/48", + "AllowedIPs=10.1.0.0/0x10", "AllowedIPs=192.168.1.1/32", + ) + } + + fun testIpMasqueradeTrueBoolean() { + assertEquals( + "yes" to "IPMasquerade= with a true boolean is deprecated; systemd treats it as ipv4, not both. Use ipv4 or both.", + single("a.network", "[Network]\nIPMasquerade=yes") + ) + // A false boolean silently means "no". + assertSilent("a.network", "Network", "IPMasquerade=no", "IPMasquerade=off", "IPMasquerade=ipv4", "IPMasquerade=both") + } + + fun testGrammarDeprecatedTokensAreReportedOnBothEngines() { + // Formerly a weak-warning annotator behind the new-engine flag; now a WARNING regardless of the flag. + for (flag in listOf(false, true)) { + ExperimentalSettings.getInstance(project).state.useGrammarParseEngine = flag + assertEquals("AF_DECnet", single("a.service", "[Service]\nRestrictAddressFamilies=AF_INET AF_DECnet").first) + assertEquals("v4", single("a.network", "[Network]\nDHCP=v4").first) + assertSilent("a.service", "Service", "RestrictAddressFamilies=AF_INET AF_INET6") + } + } + + fun testHighlightFollowsWhitespaceAndContinuationLines() { + // valueText trims and joins continuation lines; the highlight must still land on the offending characters. + assertEquals("none", single("a.service", "[Service]\nKillMode = none").first) + assertEquals("\\q", single("a.service", "[Service]\nExecStart = /bin/echo \\q").first) + assertEquals("\\\$", single("a.service", "[Service]\nExecStart=/bin/sh -c \"foo \\\n bar \\\$x\"").first) + assertEquals("\\\$", single("a.service", "[Service]\nExecStart=/bin/sh -c \"foo \\\n# comment\n bar \\\$x\"").first) + assertEquals("\\q", single("a.service", "[Service]\nExecStart=/bin/echo \\\n \\q").first) + } + + fun testWireGuardAllowedIpsIpv4Mapped() { + // sd-check: "Specified address '::ffff:10.0.0.1/100' is not properly masked, assuming '::ffff:0.0.0.0/100'." + assertTrue(single("a.netdev", "[WireGuardPeer]\nAllowedIPs=::ffff:10.0.0.1/100").second.contains("systemd uses ::ffff:0.0.0.0/100")) + assertSilent("a.netdev", "WireGuardPeer", "AllowedIPs=::ffff:10.0.0.0/104") + } + + fun testInvalidValueIsNotAlsoWarned() { + // The value is invalid (InvalidValue reports it), so no deprecation is piled on top. + assertSilent("a.service", "Service", "RestrictAddressFamilies=AF_DECnet AF_BOGUS", "PIDFile=/var/run/../x.pid") + assertSilent("a.network", "Network", "Address=10.0.0.300") + } +} diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt index e60b4482..d5674732 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/grammar/DeprecationsTest.kt @@ -38,11 +38,12 @@ class DeprecationsTest { @Test fun testIpMasqueradeBooleanValuesAreDeprecated() { - // A second, unrelated user of the same layer: the legacy boolean forms of IPMasquerade= are - // accepted but deprecated in favour of ipv4/ipv6/both/no. + // A second, unrelated user of the same layer: a true boolean for IPMasquerade= is accepted but + // deprecated in favour of ipv4/both. A false boolean is silent: systemd only warns when r > 0. + // https://github.com/systemd/systemd/blob/0f0682b773/src/network/networkd-util.c#L176-L185 val ipMasquerade = ConfigParseIpMasqueradeOptionValue().combinator assertTrue(ipMasquerade.deprecatedTokens("true").single().message.contains("ipv4")) - assertTrue(ipMasquerade.deprecatedTokens("off").single().message.contains("no")) + assertTrue(ipMasquerade.deprecatedTokens("off").isEmpty()) assertTrue(ipMasquerade.deprecatedTokens("ipv4").isEmpty()) assertTrue(ipMasquerade.deprecatedTokens("both").isEmpty()) } From 21d5d944f4040a4af7525d902fc20f64213554d1 Mon Sep 17 00:00:00 2001 From: Steve Ramage Date: Sun, 25 Oct 2026 12:31:44 -0700 Subject: [PATCH 2/2] fix: flag values systemd rejects when the unit or container starts (#565) Values that systemd's parser accepts but that are rejected or dropped at unit or container start are errors again: - unknown RestrictFileSystems= filesystem names (dropped by the BPF restrict-fs code, so the entry neither allows nor denies anything) - nspawn Bind=/BindReadOnly= with unknown mount options (container fails to start) - nspawn AmbientCapability=all (refused by systemd-nspawn) - traffic-control limits networkd refuses (e.g. ETS bands above 15, GRED and Xfrm limits) nspawn SystemCallFilter= entries with "~" after the first word are silently ignored by systemd-nspawn and now get a warning. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../optionvalues/UIntOptionValues.kt | 65 +++++++---- .../ai/ConfigParseBindOptionValue.kt | 100 ++++++++++++----- .../ai/ConfigParseCapabilityOptionValue.kt | 66 +++++++++--- .../ai/ConfigParseEtsPrioOptionValue.kt | 33 ++++-- .../ai/ConfigParseGredU32OptionValue.kt | 59 +++++++--- ...nfigParseRestrictFilesystemsOptionValue.kt | 102 ++++++++++++++---- .../ai/ConfigParseSyscallFilterOptionValue.kt | 49 +++++++-- .../ai/ConfigParseBindOptionValueTest.kt | 19 ++-- .../ConfigParseCapabilityOptionValueTest.kt | 25 +++++ .../ai/ConfigParseEtsPrioOptionValueTest.kt | 42 +++++++- .../ai/ConfigParseGredU32OptionValueTest.kt | 32 ++++-- ...ParseRestrictFilesystemsOptionValueTest.kt | 54 +++++++--- ...ConfigParseSyscallFilterOptionValueTest.kt | 22 ++-- .../ai/Issue568NetworkValidatorsTest.kt | 34 +++++- 14 files changed, 560 insertions(+), 142 deletions(-) diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/UIntOptionValues.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/UIntOptionValues.kt index 7adfa24b..9c2e5acd 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/UIntOptionValues.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/UIntOptionValues.kt @@ -1,6 +1,12 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues +import com.intellij.codeInspection.ProblemsHolder +import com.intellij.psi.util.PsiTreeUtil +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFileSectionGroups +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.FileClass import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.Validator +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.fileClass import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.* /* @@ -14,29 +20,16 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * `+5` are accepted (sd-check at 0f0682b773), and anything above the type maximum is -ERANGE. See * [SafeAtoTerminal] for the exact syntax. * - * Lenient on purpose: `[Xfrm] InterfaceId=0` is accepted by the parser and only refused later by the - * netdev verify step ("Xfrm interface ID cannot be zero"), which is not a parser rejection (#565). + * `[Xfrm] InterfaceId=0` is accepted by the parser, but xfrm_verify() then refuses it ("Xfrm interface ID cannot + * be zero", src/network/netdev/xfrm.c#L30-L39) and netdev_load_one() drops the whole .netdev + * (src/network/netdev/netdev.c#L1110-L1114), so it is flagged for that one key (#565, [Uint32OptionValue]). */ -class UIntOptionValues(validatorName: String, grammar: Combinator) : GrammarOptionValue(validatorName, grammar) { +open class UIntOptionValues(validatorName: String, grammar: Combinator) : GrammarOptionValue(validatorName, grammar) { companion object { val validators = mapOf( - Validator("config_parse_uint32", "0") to UIntOptionValues( - "config_parse_uint32", - SequenceCombinator( - SafeAtoTerminal( - signed = false, - base = 0, - flags = emptySet(), - minInclusive = 0L, - maxInclusive = 4_294_967_295L, - acceptsWhitespace = true, - acceptsMinus = true, - ), - EOF(), - ), - ), + Validator("config_parse_uint32", "0") to Uint32OptionValue(), Validator("config_parse_uint16", "0") to UIntOptionValues( "config_parse_uint16", SequenceCombinator( @@ -70,3 +63,39 @@ class UIntOptionValues(validatorName: String, grammar: Combinator) : GrammarOpti ) } } + +/** + * config_parse_uint32: 0…4294967295 for every key, except `[Xfrm] InterfaceId=` in a .netdev file, which must + * also not be 0 (xfrm_verify(), see [UIntOptionValues]). The key check is explicit because the registry is keyed + * only by (parser, ltype). + */ +private class Uint32OptionValue : UIntOptionValues("config_parse_uint32", uint32Grammar(minInclusive = 0L)) { + override fun generateProblemDescriptors(property: UnitFilePropertyType, holder: ProblemsHolder) { + val section = PsiTreeUtil.getParentOfType(property, UnitFileSectionGroups::class.java)?.sectionName + if (section == "Xfrm" && property.key == "InterfaceId" && property.containingFile.fileClass() == FileClass.NETDEV) { + XFRM_INTERFACE_ID.generateProblemDescriptors(property, holder) + } else { + super.generateProblemDescriptors(property, holder) + } + } + + private companion object { + val XFRM_INTERFACE_ID = object : GrammarOptionValue("config_parse_uint32", uint32Grammar(minInclusive = 1L)) { + override fun invalidValueMessage(key: String, badValue: String): String = + "$key must be 1…4294967295; systemd-networkd refuses 0 (\"Xfrm interface ID cannot be zero\") and does not create this netdev." + } + } +} + +private fun uint32Grammar(minInclusive: Long) = SequenceCombinator( + SafeAtoTerminal( + signed = false, + base = 0, + flags = emptySet(), + minInclusive = minInclusive, + maxInclusive = 4_294_967_295L, + acceptsWhitespace = true, + acceptsMinus = true, + ), + EOF(), +) diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseBindOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseBindOptionValue.kt index 8a377247..e240c7d5 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseBindOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseBindOptionValue.kt @@ -1,5 +1,9 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import com.intellij.codeInspection.ProblemsHolder +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.Combinator +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.GrammarOptionValue import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.COLON @@ -26,16 +30,14 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/basic/extract-word.c#L240-L281 extract_many_words (":", EXTRACT_DONT_COALESCE_SEPARATORS) * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/basic/extract-word.c#L10-L188 extract_first_word * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/nspawn/nspawn-mount.c#L759-L800 parse_mount_bind_options (only at container start, NOT at parse time) + * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/nspawn/nspawn-mount.c#L813-L816 mount_bind returns its error, and mount_custom (#L1076-L1101) aborts the container start * oracle steve-local/systemd-tools/bin/sd-check (fuzz-nspawn-settings at 0f0682b773): `Bind=/a:/b:bogus`, `Bind=/a:/b:rbind:x`, * `BindReadOnly=/a:/b:ro`, `Bind=/a:/b:rbind,bogus`, `Bind=/a:/b::`, `Bind=/a:/b:has space` are all accepted - * (no output); `Bind=/a:relative:rbind` → "Invalid bind mount specification ... Invalid argument". + * (no output; the container then fails to start, #565); `Bind=/a:relative:rbind` → "Invalid bind mount specification ... Invalid argument". * tests https://github.com/systemd/systemd/blob/0f0682b773/test/units/TEST-13-NSPAWN.nspawn.sh#L264-L287 */ -/** - * The options parse_mount_bind_options() knows. Listed first only for completion and highlighting: anything - * else is accepted too (see BIND_OPTIONS_FIELD), because only the container start rejects it. - */ +/** The options parse_mount_bind_options() knows. Listed for completion and highlighting. */ private val BIND_OPTION = FlexibleLiteralChoiceTerminal("rbind", "norbind", "idmap", "noidmap", "rootidmap", "owneridmap") private val COMMA = LiteralChoiceTerminal(",") @@ -51,20 +53,65 @@ private val BIND_OPTIONS = SequenceCombinator( ) /** - * Everything after the second colon. bind_mount_parse() strdup()s it verbatim (no unescaping, no checks), so - * any text is accepted at parse time. The known-option list is tried first so that a valid list is coloured - * and completed as options; otherwise the whole rest is taken as one opaque option string. - * parse_mount_bind_options() rejects unknown options only when the container starts ("Invalid bind mount - * option"); GitHub issue #565 tracks a separate warning for those. + * Everything after the second colon, for colouring and completion and for locating path errors. + * bind_mount_parse() strdup()s it verbatim (no unescaping, no checks), so the settings parser accepts any + * text. The known-option list is tried first so that a valid list is coloured and completed as options; + * otherwise the whole rest is taken as one opaque option string. Unknown options are checked separately + * with [BIND_OPTIONS_STRICT]. */ private val BIND_OPTIONS_FIELD = PreferValidAlternativeCombinator( SequenceCombinator(BIND_OPTIONS, EOF()), SequenceCombinator(RegexTerminal("""[\s\S]+""", """[\s\S]+"""), EOF()), ) +/** + * One comma-separated word of the option string, valid only if parse_mount_bind_options() knows it. Any other + * word (`bogus`, `ro`, `rbind:x`, `has space`, `:`) makes it fail with "Invalid bind mount option" when the + * container starts (nspawn-mount.c#L789-L791), and the container does not start. + */ +private val BIND_OPTION_WORD_STRICT = RegexTerminal( + """[^,]+""", + """(?:rbind|norbind|idmap|noidmap|rootidmap|owneridmap)(?=,|\z)""", +) + +/** The option string as parse_mount_bind_options() reads it, with every word checked. */ +private val BIND_OPTIONS_STRICT = SequenceCombinator( + ZeroOrMore(COMMA), + ZeroOrOne(SequenceCombinator(BIND_OPTION_WORD_STRICT, ZeroOrMore(SequenceCombinator(OneOrMore(COMMA), BIND_OPTION_WORD_STRICT)))), + ZeroOrMore(COMMA), + EOF(), +) + /** A lone path is both source and destination, so it must be absolute (optionally `+`-prefixed). */ private val SINGLE_PATH = RegexTerminal("""(?:[^:\\]|\\[\s\S])+""", """\+?\\?/(?:[^:\\]|\\[\s\S])*""") +/** `[SOURCE:]DEST[:OPTIONS]` or a lone `PATH`, with [optionsField] after the second colon. */ +private fun bindGrammar(optionsField: Combinator) = AlternativeCombinator( + SequenceCombinator( + ZeroOrOne(COLON_FIELD_SOURCE_PATH), + COLON, + COLON_FIELD_ABSOLUTE_PATH, + AlternativeCombinator( + SequenceCombinator(COLON, optionsField), + EOF(), + ), + ), + SequenceCombinator(SINGLE_PATH, EOF()), +) + +/** + * The same value with every option checked. Only consulted once the paths are known to be valid, so any + * semantic error it finds is an unknown option, and the message can say so. + */ +private val BIND_WITH_STRICT_OPTIONS = object : GrammarOptionValue("config_parse_bind", bindGrammar(BIND_OPTIONS_STRICT)) { + override fun invalidValueMessage(key: String, badValue: String): String { + val readOnlyHint = if (badValue == "ro" || badValue == "rw") " For a read-only mount use BindReadOnly= instead." else "" + return "$key: '$badValue' is not a bind mount option, so systemd-nspawn refuses to start the container " + + "(\"Invalid bind mount option\"). Options are a comma-separated list of rbind, norbind, idmap, noidmap, " + + "rootidmap, owneridmap.$readOnlyHint" + } +} + /** * Validator for the .nspawn `[Files] Bind=` and `BindReadOnly=` settings (config_parse_bind; ltype 1 * only sets the read-only flag, so both share one grammar). @@ -80,24 +127,21 @@ private val SINGLE_PATH = RegexTerminal("""(?:[^:\\]|\\[\s\S])+""", """\+?\\?/(? * other path (a relative one is resolved against the working directory). DEST must be absolute. * `SOURCE:` (an empty DEST) is invalid; an empty OPTIONS after the second colon is ignored. * - * OPTIONS is not inspected at config-parse time, so any option string is accepted here. Only when the - * container starts does parse_mount_bind_options() refuse anything other than a comma-separated list of - * `rbind`, `norbind`, `idmap`, `noidmap`, `rootidmap`, `owneridmap` (e.g. `bogus`, `rbind:x`, `ro`); this - * validator follows the parser and accepts them, and GitHub issue #565 tracks a separate warning for - * such runtime-only rejections. Parse errors are logged and the setting ignored. + * The settings parser stores OPTIONS unchecked, but when the container starts parse_mount_bind_options() + * refuses anything other than a comma-separated list of `rbind`, `norbind`, `idmap`, `noidmap`, `rootidmap`, + * `owneridmap` (e.g. `bogus`, `rbind:x`, `ro`) and the container fails to start, so those are flagged too + * (#565). The paths are checked first, with the lenient [BIND_OPTIONS_FIELD]; only a value whose paths are + * fine is then checked with [BIND_WITH_STRICT_OPTIONS], so each value gets at most one problem. */ class ConfigParseBindOptionValue : SimpleGrammarOptionValues( "config_parse_bind", - AlternativeCombinator( - SequenceCombinator( - ZeroOrOne(COLON_FIELD_SOURCE_PATH), - COLON, - COLON_FIELD_ABSOLUTE_PATH, - AlternativeCombinator( - SequenceCombinator(COLON, BIND_OPTIONS_FIELD), - EOF(), - ), - ), - SequenceCombinator(SINGLE_PATH, EOF()), - ) -) + bindGrammar(BIND_OPTIONS_FIELD), +) { + override fun generateProblemDescriptors(property: UnitFilePropertyType, holder: ProblemsHolder) { + val before = holder.resultCount + super.generateProblemDescriptors(property, holder) + if (holder.resultCount == before) { + BIND_WITH_STRICT_OPTIONS.generateProblemDescriptors(property, holder) + } + } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseCapabilityOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseCapabilityOptionValue.kt index 287d1665..21f9781e 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseCapabilityOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseCapabilityOptionValue.kt @@ -1,5 +1,8 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import com.intellij.codeInspection.ProblemsHolder +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.GrammarOptionValue import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.CAPABILITY_NAME @@ -25,21 +28,51 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * linux https://man7.org/linux/man-pages/man7/capabilities.7.html */ +/** A capability number (0…CAP_LIMIT) read by capability_from_name()'s safe_atoi(). */ +private val CAPABILITY_NUMBER = SafeAtoTerminal( + signed = true, + base = 0, + flags = emptySet(), + minInclusive = 0L, + maxInclusive = 62L, + acceptsWhitespace = false, + acceptsMinus = true, +) + /** One capability word: the literal `all`, a capability number, or a capability name. */ private val CAPABILITY_WORD = AlternativeCombinator( LiteralChoiceTerminal("all"), - SafeAtoTerminal( - signed = true, - base = 0, - flags = emptySet(), - minInclusive = 0L, - maxInclusive = 62L, - acceptsWhitespace = false, - acceptsMinus = true, - ), + CAPABILITY_NUMBER, CAPABILITY_NAME, ) +/** An AmbientCapability= word: no `all` (it falls to [CAPABILITY_NAME], which does not know it). */ +private val AMBIENT_CAPABILITY_WORD = AlternativeCombinator( + CAPABILITY_NUMBER, + CAPABILITY_NAME, +) + +/** + * AmbientCapability=: the settings parser takes `all`, but verify_arguments() then refuses it and the + * container does not start ("AmbientCapability= does not support the value all.", src/nspawn/nspawn.c#L1559-L1561). + */ +private val AMBIENT_CAPABILITY = object : GrammarOptionValue( + "config_parse_capability", + SequenceCombinator( + AMBIENT_CAPABILITY_WORD, + ZeroOrMore(SequenceCombinator(WhitespaceTerminal(), AMBIENT_CAPABILITY_WORD)), + EOF(), + ), +) { + override fun invalidValueMessage(key: String, badValue: String): String = + if (badValue == "all") { + "$key=all is refused by systemd-nspawn (\"AmbientCapability= does not support the value all\") and the container " + + "does not start. List the capabilities explicitly, e.g. CAP_NET_ADMIN CAP_SYS_TIME; each must also be in Capability=." + } else { + super.invalidValueMessage(key, badValue) + } +} + /** * Validator for the .nspawn `[Exec] Capability=`, `AmbientCapability=` and `DropCapability=` settings, * all of which use config_parse_capability with ltype 0. @@ -55,9 +88,9 @@ private val CAPABILITY_WORD = AlternativeCombinator( * * An unknown word is logged and skipped rather than failing the whole line; it is still flagged here. * - * Deliberately lenient: the man page and nspawn itself refuse `all` for AmbientCapability= — that check - * happens later, in verify_arguments() ("AmbientCapability= does not support the value all"), and the - * grammar is shared by all three keys via (parser, ltype), so it cannot single that key out. + * AmbientCapability= also refuses `all`: the settings parser stores it, but verify_arguments() then fails + * ("AmbientCapability= does not support the value all"), so the container does not start (#565). The three + * keys share (parser, ltype), so [generateProblemDescriptors] picks the stricter grammar for that key. */ class ConfigParseCapabilityOptionValue : SimpleGrammarOptionValues( "config_parse_capability", @@ -66,4 +99,11 @@ class ConfigParseCapabilityOptionValue : SimpleGrammarOptionValues( ZeroOrMore(SequenceCombinator(WhitespaceTerminal(), CAPABILITY_WORD)), EOF(), ) -) +) { + override fun generateProblemDescriptors(property: UnitFilePropertyType, holder: ProblemsHolder) { + when (property.key) { + "AmbientCapability" -> AMBIENT_CAPABILITY.generateProblemDescriptors(property, holder) + else -> super.generateProblemDescriptors(property, holder) + } + } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseEtsPrioOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseEtsPrioOptionValue.kt index 2e27d6cb..e483aad2 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseEtsPrioOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseEtsPrioOptionValue.kt @@ -24,7 +24,8 @@ private val ETS_BAND = SafeAtoTerminal( base = 0, flags = emptySet(), minInclusive = BigInteger.ZERO, - maxInclusive = SafeAtoTerminal.UINT8_MAX, + // safe_atou8 allows up to 255, but see the class comment: only 0…15 can ever be below the band count. + maxInclusive = BigInteger.valueOf(15), acceptsWhitespace = false, acceptsMinus = true, ) @@ -40,12 +41,32 @@ private val ETS_BAND = SafeAtoTerminal( * cannot contain a blank: `0b+1` is 1, but `0b 1` is the words `0b` (invalid) and `1`. Out-of-range or unparsable entries are logged and skipped by * the C, and so are flagged here; so is a 17th entry. * - * Lenient: enhanced_transmission_selection_verify later rejects the whole section if any entry is not - * below the number of bands (at most 16), so values 16…255 can never actually work; they are accepted - * here because the man page documents 0…255 and that is what the parser itself checks. The 16-entry - * cap is cumulative across lines; only the per-line count is checked. + * The parser itself takes 0…255 (and the man page says so), but enhanced_transmission_selection_verify() + * drops the whole section when an entry is not below the number of bands (src/network/tc/ets.c#L325-L330), + * and an ETS qdisc has at most 16 bands (Bands= is capped at TCQ_ETS_MAX_BANDS, ets.c#L131, and the kernel + * refuses more), so 16…255 never work and are flagged (#565). The 16-entry cap is cumulative across lines; + * only the per-line count is checked. */ class ConfigParseEtsPrioOptionValue : SimpleGrammarOptionValues( "config_parse_ets_prio", SequenceCombinator(ETS_BAND, Repeat(SequenceCombinator(WhitespaceTerminal(), ETS_BAND), 0, 15), EOF()), -) +) { + // Only a well-formed number above 15 gets the band message; other bad words (`-1`, `0b`) keep the generic one. + override fun invalidValueMessage(key: String, badValue: String): String = + if ((etsNumber(badValue) ?: 0) > 15) { + "$key: band '$badValue' is out of range. Bands are numbered 0…15 (an ETS qdisc has at most 16), and systemd-networkd " + + "ignores the whole [EnhancedTransmissionSelection] section when a PriorityMap= entry is not below the number of bands." + } else { + super.invalidValueMessage(key, badValue) + } +} + +/** The value of a plain unsigned safe_atou8 word in C base-0 notation (`16`, `0x10`, `020`), or null for anything else. */ +private fun etsNumber(word: String): Long? { + val digits = word.removePrefix("+") + return when { + digits.startsWith("0x") || digits.startsWith("0X") -> digits.substring(2).toLongOrNull(16) + digits.length > 1 && digits.startsWith("0") -> digits.substring(1).toLongOrNull(8) + else -> digits.toLongOrNull(10) + }?.takeIf { it >= 0 && digits.none { c -> c == '-' || c == '+' } } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseGredU32OptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseGredU32OptionValue.kt index fbf483a5..73dbe5bf 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseGredU32OptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseGredU32OptionValue.kt @@ -1,5 +1,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import com.intellij.codeInspection.ProblemsHolder +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.* @@ -17,6 +19,34 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * tests https://github.com/systemd/systemd/blob/0f0682b773/test/test-network/conf/25-qdisc-gred.network#L12-L13 VirtualQueues=12, DefaultVirtualQueue=10 */ +/** A safe_atou32 number in [min]…[max]. */ +private fun gredNumber(min: Long, max: Long) = SequenceCombinator( + SafeAtoTerminal( + signed = false, + base = 0, + flags = emptySet(), + minInclusive = min, + maxInclusive = max, + acceptsWhitespace = true, + acceptsMinus = true, + ), + EOF(), +) + +/** VirtualQueues=: 0 always fails verify (DefaultVirtualQueue= is at least 0 and must be below it). */ +private val GRED_VIRTUAL_QUEUES = object : GrammarOptionValue("config_parse_gred_u32", gredNumber(1L, 16L)) { + override fun invalidValueMessage(key: String, badValue: String): String = + "$key must be 1…16. systemd-networkd ignores values above 16, and with 0 DefaultVirtualQueue= can never be below " + + "VirtualQueues=, so the whole [GenericRandomEarlyDetection] section is ignored." +} + +/** DefaultVirtualQueue=: must be below VirtualQueues=, which is at most 16. */ +private val GRED_DEFAULT_VIRTUAL_QUEUE = object : GrammarOptionValue("config_parse_gred_u32", gredNumber(0L, 15L)) { + override fun invalidValueMessage(key: String, badValue: String): String = + "$key must be 0…15 and below VirtualQueues= (at most 16); otherwise systemd-networkd ignores the whole " + + "[GenericRandomEarlyDetection] section." +} + /** * Validator for [GenericRandomEarlyDetection] VirtualQueues=, DefaultVirtualQueue=. * @@ -28,22 +58,19 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * assignment" (although, at 0f0682b773, it still stores the value — a C bug; the kernel refuses more than * 16 DPs anyway), so it is flagged here. * - * Lenient: both keys share the parser, so 0..16 is accepted for both, although VirtualQueues=0 and - * DefaultVirtualQueue=16 always fail generic_random_early_detection_verify (DefaultVirtualQueue= must - * be less than VirtualQueues=). The man page documents VirtualQueues= as 1…16. + * generic_random_early_detection_verify() then drops the whole section unless DefaultVirtualQueue= is below + * VirtualQueues= (src/network/tc/gred.c#L56-L65), so `VirtualQueues=0` and `DefaultVirtualQueue=16` can never + * work and are flagged per key (#565). Whether the two values fit together is not checked (that needs both lines). */ class ConfigParseGredU32OptionValue : SimpleGrammarOptionValues( "config_parse_gred_u32", - SequenceCombinator( - SafeAtoTerminal( - signed = false, - base = 0, - flags = emptySet(), - minInclusive = 0L, - maxInclusive = 16L, - acceptsWhitespace = true, - acceptsMinus = true, - ), - EOF(), - ), -) + gredNumber(0L, 16L), +) { + override fun generateProblemDescriptors(property: UnitFilePropertyType, holder: ProblemsHolder) { + when (property.key) { + "VirtualQueues" -> GRED_VIRTUAL_QUEUES.generateProblemDescriptors(property, holder) + "DefaultVirtualQueue" -> GRED_DEFAULT_VIRTUAL_QUEUE.generateProblemDescriptors(property, holder) + else -> super.generateProblemDescriptors(property, holder) + } + } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseRestrictFilesystemsOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseRestrictFilesystemsOptionValue.kt index e2884df5..3c2c1186 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseRestrictFilesystemsOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseRestrictFilesystemsOptionValue.kt @@ -6,7 +6,13 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.FILESYSTEM_GROUP_NAME import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.FILESYSTEM_NAME import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.LiteralChoiceTerminal -import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.RegexTerminal +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.MatchResult +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.NoMatch +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.Parse +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ParseStep +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ParsedToken +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.Stuck +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.TerminalCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.SequenceCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.WHITESPACE_OR_COMMA_SEPARATOR import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.ZeroOrMore @@ -20,14 +26,15 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/basic/extract-word.c#L10 extract_first_word (WHITESPACE ",", EXTRACT_UNQUOTE) * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/core/bpf-restrict-fs.c#L212 bpf_restrict_fs_parse_filesystem * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/basic/filesystems.c#L21 filesystem_set_find (group names) - * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/core/bpf-restrict-fs.c#L117 fs_type_from_string at unit start ("Invalid filesystem name, ignoring"), runtime only + * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/core/bpf-restrict-fs.c#L116-L121 fs_type_from_string at unit start ("Invalid filesystem name, ignoring"), via exec-invoke.c#L2123 * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/basic/filesystem-sets.py#L9 NAME_TO_MAGIC / FILESYSTEM_SETS (the name tables) * tests https://github.com/systemd/systemd/blob/0f0682b773/test/units/TEST-07-PID1.exec-context.sh#L351-L358 ("foo", "@foo" are logged and ignored) * linux https://man7.org/linux/man-pages/man2/statfs.2.html (the f_type magics the names map to) * - * sd-check at 0f0682b773 (one value per file): accepted `EXT4`, `ext5`, `invalid`, `temporary`, + * sd-check at 0f0682b773 (one value per file): parse-time silent for `EXT4`, `ext5`, `invalid`, `temporary`, * `~proc,sysfs,foo`, `@auxiliary-api btrfs nfs @temporary unknown`, `ext"4"`, `ext4""`, `""`, `"a\"b"`, - * `ext4 ~proc`, `ext4 ~@foo`, `~ ~@bogus`, `"~@bogus"`, `\~proc`, `foo.bar!`; refused `@unknowngroup`, `@`, + * `ext4 ~proc`, `ext4 ~@foo`, `~ ~@bogus`, `"~@bogus"`, `\~proc`, `foo.bar!` (all but `ext"4"` / `ext4""` name no + * file system and are dropped with a warning when the unit starts, so they are flagged #565); refused `@unknowngroup`, `@`, * `@Basic-api`, `~@bogus`, `"@foo"`, `""@foo`, `\@foo`, `"@basic-api"x` ("Unknown filesystem group") and * `ext"4`, `ext4 "unterminated` ("Trailing garbage"). */ @@ -66,24 +73,71 @@ private val KNOWN_FILESYSTEM_NAME = LiteralChoiceTerminal(*FILESYSTEM_NAME.choic private val KNOWN_NAME_WORD = """(?["']?)(?:""" + KNOWN_FILESYSTEM_NAME.choices.joinToString("|") { Regex.escape(it) } + """)\k(?=[\s,]|\z)""" +/** + * A word that names one file system: unquoted with EXTRACT_UNQUOTE (quotes removed, `\x` read as `x`), it + * must be one of the known names. The span is taken by [syntactic]; the word is valid only if it unquotes to a + * [known] name. The unit-file parser stores such words unchecked, but when the unit starts the BPF code looks + * each one up and logs "bpf-restrict-fs: Invalid filesystem name '%s', ignoring." for an unknown one + * (src/core/bpf-restrict-fs.c#L116-L121, called from exec-invoke.c#L2123), so the restriction silently loses that entry: flagged as invalid. + */ +private class FilesystemNameWordTerminal(syntactic: String, private val known: Set) : TerminalCombinator { + private val syntacticRegex = syntactic.toRegex() + + private fun span(value: String, offset: Int): String? = syntacticRegex.matchAt(value, offset)?.value + + private fun matched(text: String, offset: Int) = MatchResult(listOf(text), offset + text.length, listOf(this), offset + text.length) + + override fun SyntacticMatch(value: String, offset: Int): MatchResult = span(value, offset)?.let { matched(it, offset) } ?: NoMatch + + override fun SemanticMatch(value: String, offset: Int): MatchResult = + span(value, offset)?.takeIf { unquote(it) in known }?.let { matched(it, offset) } ?: NoMatch + + override fun parse(value: String, offset: Int): Sequence { + val text = span(value, offset) ?: return sequenceOf(Stuck(offset, setOf(this))) + return sequenceOf(Parse(offset + text.length, listOf(ParsedToken(offset, offset + text.length, text, this, unquote(text) in known)))) + } + + override fun toString(): String = "FilesystemNameWord(\"${syntacticRegex.pattern}\")" +} + +/** The word as extract_first_word(…, EXTRACT_UNQUOTE) returns it: quotes dropped, `\x` read as `x`. */ +private fun unquote(word: String): String { + val out = StringBuilder() + var quote: Char? = null + var i = 0 + while (i < word.length) { + val c = word[i] + when { + c == '\\' && i + 1 < word.length -> { out.append(word[i + 1]); i++ } + quote == null && (c == '"' || c == '\'') -> quote = c + c == quote -> quote = null + else -> out.append(c) + } + i++ + } + return out.toString() +} + +private val KNOWN_FILESYSTEM_NAMES: Set = FILESYSTEM_NAME.choices.toSet() + /** * Any word the unit-file parser stores as a plain file system name, i.e. one that does not unquote to a - * leading `@`. The parser keeps such names unchecked: an unknown one (`EXT4`, `ext5`, `foo`) is only logged - * and skipped when the unit starts and the BPF map is filled ("bpf-restrict-fs: Invalid filesystem name '%s', - * ignoring.", src/core/bpf-restrict-fs.c#L117-L121), so it is accepted here. Issue #565 tracks a separate - * warning for such names. A word that is wholly empty quotes (`""`) is stored as the empty name and accepted. + * leading `@`. Valid only when it unquotes to a known name (`ext"4"`, `\ext4`); an unknown one (`EXT4`, + * `ext5`, `foo`) is skipped at unit start, see [FilesystemNameWordTerminal]. Exact known words are left to + * [KNOWN_FILESYSTEM_NAME] so each word has a single parse. A word that is wholly empty quotes (`""`) is the + * empty name, which is not a file system either. */ -private val ANY_FILESYSTEM_NAME = RegexTerminal( - "(?!$KNOWN_NAME_WORD)(?:(?:\"\"|'')+(?:(?:$FIRST_PIECE_NO_AT)(?:$WORD_PIECE)*)?|(?:$FIRST_PIECE_NO_AT_NO_TILDE)(?:$WORD_PIECE)*)", +private val ANY_FILESYSTEM_NAME = FilesystemNameWordTerminal( "(?!$KNOWN_NAME_WORD)(?:(?:\"\"|'')+(?:(?:$FIRST_PIECE_NO_AT)(?:$WORD_PIECE)*)?|(?:$FIRST_PIECE_NO_AT_NO_TILDE)(?:$WORD_PIECE)*)", + KNOWN_FILESYSTEM_NAMES, ) /** * A word starting with a raw `~` anywhere except the very start of the value. Only `rvalue[0]` is the - * deny-list marker, so `ext4 ~proc` or `~ ~@bogus` store the literal name `~proc` / `~@bogus`, which never - * matches a file system at unit start. Accepted for the same reason as [ANY_FILESYSTEM_NAME] (#565). + * deny-list marker, so `ext4 ~proc` or `~ ~@bogus` store the literal name `~proc` / `~@bogus`, which no file + * system has: skipped at unit start like any unknown name, so always invalid. */ -private val TILDE_WORD = RegexTerminal("~(?:$WORD_PIECE)*", "~(?:$WORD_PIECE)*") +private val TILDE_WORD = FilesystemNameWordTerminal("~(?:$WORD_PIECE)*", KNOWN_FILESYSTEM_NAMES) /** A word that does not start with a raw `~`: an unknown name, a known name or `@group`, either possibly quoted. */ private val ENTRY_NOT_TILDE = AlternativeCombinator( @@ -116,10 +170,10 @@ private val ENTRY = AlternativeCombinator(TILDE_WORD, ENTRY_NOT_TILDE) * - A word that unquotes to `@group` must be one of filesystem-sets.py's FILESYSTEM_SETS (`@basic-api`, * `@common-block`, `@network`, `@known`, …). The parser refuses an unknown group ("Unknown filesystem * group, ignoring"), so it is flagged. The lookup is exact and case-sensitive. - * - Any other word is a file system name. The parser stores it unchecked. An unknown name (`EXT4`, `ext5`, - * `foo`, a literal `~proc` after the first word) is only rejected when the unit starts ("Invalid - * filesystem name, ignoring"), so it is accepted here. Issue #565 tracks a separate inspection that - * warns about values like this. Known names stay a separate alternative for completion and coloring. + * - Any other word is a file system name. The parser stores it unchecked, but an unknown name (`EXT4`, + * `ext5`, `foo`, a literal `~proc` after the first word) is dropped when the unit starts ("Invalid + * filesystem name, ignoring"), so the entry has no effect: flagged (#565). The lookup is + * exact and case-sensitive. Known names stay a separate alternative for completion and coloring. * * Known over-rejection: a word that only becomes `@group` through escaping or partial quoting * (`\@basic-api`, `'@'basic-api`, `""@basic-api`) is accepted by systemd but flagged here; `"@basic-api"` @@ -169,4 +223,16 @@ class ConfigParseRestrictFilesystemsOptionValue : SimpleGrammarOptionValues( EOF(), ), ) -) +) { + // The bad token of an unknown `@group` is the name after the `@`, which looks just like a bad file system + // name, so one message covers both. + override fun invalidValueMessage(key: String, badValue: String): String = + if (badValue.startsWith("~")) { + "$key: '$badValue' is not a file system. '~' only means \"deny list\" as the very first character of the value; " + + "here it is part of the name, so systemd ignores this entry when the unit starts." + } else { + "$key: unknown file system or group '${unquote(badValue)}'; systemd ignores this entry, so it neither allows nor denies anything. " + + "Names are case-sensitive kernel file system types (ext4, btrfs, tmpfs, ...); " + + "groups are ${FILESYSTEM_GROUP_NAME.choices.sorted().joinToString(", ") { "@$it" }}." + } +} diff --git a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSyscallFilterOptionValue.kt b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSyscallFilterOptionValue.kt index a8fc03a2..1b1a753a 100644 --- a/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSyscallFilterOptionValue.kt +++ b/src/main/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/semanticdata/optionvalues/ai/ConfigParseSyscallFilterOptionValue.kt @@ -1,5 +1,11 @@ package net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.ai +import com.intellij.codeInspection.ProblemHighlightType +import com.intellij.codeInspection.ProblemsHolder +import com.intellij.openapi.util.TextRange +import net.sjrx.intellij.plugins.systemdunitfiles.psi.UnitFilePropertyType +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.FileClass +import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.fileClass import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.SimpleGrammarOptionValues import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.AlternativeCombinator import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.grammar.Combinator @@ -27,7 +33,8 @@ import net.sjrx.intellij.plugins.systemdunitfiles.semanticdata.optionvalues.gram * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/basic/parse-util.c#L342 parse_errno * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/shared/seccomp-util.c#L1293 seccomp_parse_syscall_filter * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/shared/seccomp-util.c#L334 syscall_filter_sets[] (group names) - * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/shared/seccomp-util.c#L1035 seccomp_add_syscall_filter_item (nspawn's consumer) + * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/shared/seccomp-util.c#L1035 seccomp_add_syscall_filter_item (nspawn's consumer; unknown names dropped with log_debug, #L1069-L1075) + * leaf https://github.com/systemd/systemd/blob/0f0682b773/src/nspawn/nspawn-seccomp.c#L152-L156 nspawn applies the allow list at container start * tests https://github.com/systemd/systemd/blob/0f0682b773/src/test/test-seccomp.c#L56 TEST(parse_syscall_and_errno) * tests https://github.com/systemd/systemd/blob/0f0682b773/src/test/test-parse-util.c#L729 TEST(parse_errno) * linux https://man7.org/linux/man-pages/man2/syscalls.2.html @@ -50,14 +57,14 @@ private val NAME: Combinator = SYSCALL_OR_GROUP * `rvalue[0]` is the deny-list marker in both parsers, so this word is taken literally as the name `~@mount`. * * - nspawn's parser stores it unchecked; at container start seccomp_add_syscall_filter_item() cannot resolve - * it and drops it with a debug message. That is a runtime-only rejection, so it is accepted here; issue #565 - * tracks a separate warning for it. + * it and drops it with only a debug message, so the entry silently does nothing. The grammar accepts it and + * [ConfigParseSyscallFilterOptionValue.generateProblemDescriptors] adds a weak warning in .nspawn files (#565). * - the unit-file parser hands it to libseccomp, which does not know it ("System call ~@mount is not known, * ignoring."). That is the same unknown-name warning this grammar already leaves to the shape check for * `madeupcall`. * * The validator registry is keyed only by (parser, ltype), and both files register (config_parse_syscall_filter, - * 0), so the two cannot be told apart; the lenient nspawn behaviour wins. After the `~` the name is not looked + * 0), so the grammar takes the lenient nspawn behaviour; the file type decides about the warning. After the `~` the name is not looked * up as a group by either parser, so any group-shaped word is accepted (`~@bogus`). */ private val TILDE_NAME: Combinator = SequenceCombinator( @@ -94,7 +101,7 @@ private val DENY_ENTRY: Combinator = SequenceCombinator( * letters, digits, `_`); `DONOTEXIST` is flagged, `madeupcall` is not. * - a word starting with `~` after the first position (`@system-service ~@mount`), which both parsers take * as a literal, never-matching name. nspawn only drops it at container start, so it is accepted (see - * [TILDE_NAME]; issue #565 tracks a warning for it). + * [TILDE_NAME]); in .nspawn files it gets a weak warning, because the entry is silently ignored (#565). * - in deny-list mode only, any of those followed by `:` and an errno name (case-insensitive, * `EPERM`), a number 0…4095 (decimal, `0x` hex or `0` octal) or `kill`. parse_syscall_and_errno * rejects an empty name (`:EPERM`), an empty or junk errno (`uname:`, `uname:EINVALaaa`) and an @@ -147,4 +154,34 @@ class ConfigParseSyscallFilterOptionValue : SimpleGrammarOptionValues( EOF(), ), ) -) +) { + /** + * In a .nspawn file, also warns about each word that starts with `~` after the first character of the value: + * nspawn's parser stores it as the literal name `~@mount`, which seccomp_add_syscall_filter_item() cannot + * resolve at container start and drops with only a debug message (seccomp-util.c#L1069-L1075), so the entry + * silently does nothing. Unit files are left to the grammar alone (the same name check as `madeupcall`). + */ + override fun generateProblemDescriptors(property: UnitFilePropertyType, holder: ProblemsHolder) { + val before = holder.resultCount + super.generateProblemDescriptors(property, holder) + if (holder.resultCount != before || property.containingFile.fileClass() != FileClass.NSPAWN) return + val value = property.valueText ?: return + // Words as extract_first_word(WHITESPACE ","); `rvalue[0] == '~'` is the deny-list marker, not part of a word. + val start = if (value.startsWith("~")) 1 else 0 + for (word in NSPAWN_WORD.findAll(value, start)) { + if (!word.value.startsWith("~")) continue + holder.registerProblem( + property.valueNode.psi, + "${property.key}: '${word.value}' is ignored by systemd-nspawn. '~' is only honoured as the first character " + + "of the value (making the whole line a deny list); here it is part of the name, which matches no system " + + "call. Put the calls to deny on their own ${property.key}=~... line.", + ProblemHighlightType.WEAK_WARNING, + TextRange(word.range.first, word.range.last + 1), + ) + } + } + + private companion object { + val NSPAWN_WORD = Regex("[^\\s,]+") + } +} diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseBindOptionValueTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseBindOptionValueTest.kt index 0d0c09fa..7bfedc52 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseBindOptionValueTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseBindOptionValueTest.kt @@ -2,6 +2,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.inspections.ai import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest import net.sjrx.intellij.plugins.systemdunitfiles.inspections.InvalidValueInspection +import com.intellij.lang.annotation.HighlightSeverity import org.junit.Test class ConfigParseBindOptionValueTest : AbstractUnitFileTest() { @@ -60,11 +61,11 @@ class ConfigParseBindOptionValueTest : AbstractUnitFileTest() { } @Test - fun testOptionsRejectedOnlyAtContainerStart() { - // Accepted: bind_mount_parse() stores the option string verbatim, so the settings parser (sd-check / - // fuzz-nspawn-settings at 0f0682b773) accepts all of these. parse_mount_bind_options() fails the container - // start with "Invalid bind mount option" (`ro` is not an option, read-only is BindReadOnly=; `rbind:x` is one - // word), but that is not a parse error. Issue #565 tracks a separate warning for them. + fun testUnknownOptionsFailTheContainerStart() { + // The settings parser (sd-check / fuzz-nspawn-settings at 0f0682b773) stores the option string verbatim and + // accepts all of these, but parse_mount_bind_options() refuses them with "Invalid bind mount option" + // (src/nspawn/nspawn-mount.c#L789-L791) and mount_bind()/mount_custom() abort the container start + // (#L813-L816, #L1076-L1101). `ro` is not an option (read-only is BindReadOnly=); `rbind:x` is one word (#565). // language="unit file (systemd)" val file = """ [Files] @@ -79,8 +80,12 @@ class ConfigParseBindOptionValueTest : AbstractUnitFileTest() { setupFileInEditor("file.nspawn", file) enableInspection(InvalidValueInspection::class.java) - val highlights = myFixture.doHighlighting() + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } - assertSize(0, highlights) + assertSize(7, highlights) + val bogus = highlights.first { it.text == "bogus" }.description + assertStringContains("Bind: 'bogus' is not a bind mount option, so systemd-nspawn refuses to start the container", bogus) + assertStringContains("rbind, norbind, idmap, noidmap, rootidmap, owneridmap", bogus) + assertStringContains("For a read-only mount use BindReadOnly= instead.", highlights.single { it.text == "ro" }.description) } } diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseCapabilityOptionValueTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseCapabilityOptionValueTest.kt index d655f23b..5ee59386 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseCapabilityOptionValueTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseCapabilityOptionValueTest.kt @@ -2,6 +2,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.inspections.ai import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest import net.sjrx.intellij.plugins.systemdunitfiles.inspections.InvalidValueInspection +import com.intellij.lang.annotation.HighlightSeverity import org.junit.Test class ConfigParseCapabilityOptionValueTest : AbstractUnitFileTest() { @@ -54,4 +55,28 @@ class ConfigParseCapabilityOptionValueTest : AbstractUnitFileTest() { assertSize(7, highlights) } + + @Test + fun testAmbientCapabilityAllFailsTheContainerStart() { + // The settings parser stores `all` for every key (sd-check silent), but verify_arguments() refuses it for + // AmbientCapability= only ("AmbientCapability= does not support the value all.", src/nspawn/nspawn.c#L1559-L1561), + // so the container does not start (#565). Capability=all and DropCapability=all stay valid (testValidValues). + // language="unit file (systemd)" + val file = """ + [Exec] + AmbientCapability=all + AmbientCapability=CAP_BPF all + """.trimIndent() + + setupFileInEditor("file.nspawn", file) + enableInspection(InvalidValueInspection::class.java) + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } + + assertSize(2, highlights) + for (highlight in highlights) { + assertEquals("all", highlight.text) + assertStringContains("AmbientCapability=all is refused by systemd-nspawn", highlight.description) + assertStringContains("List the capabilities explicitly", highlight.description) + } + } } diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseEtsPrioOptionValueTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseEtsPrioOptionValueTest.kt index 5796bcb5..d21f815e 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseEtsPrioOptionValueTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseEtsPrioOptionValueTest.kt @@ -2,6 +2,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.inspections.ai import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest import net.sjrx.intellij.plugins.systemdunitfiles.inspections.InvalidValueInspection +import com.intellij.lang.annotation.HighlightSeverity import org.junit.Test class ConfigParseEtsPrioOptionValueTest : AbstractUnitFileTest() { @@ -17,7 +18,7 @@ class ConfigParseEtsPrioOptionValueTest : AbstractUnitFileTest() { PriorityMap=3 4 5 PriorityMap=6 7 PriorityMap=0 - PriorityMap=255 + PriorityMap=15 PriorityMap=0x0f 017 0b1 PriorityMap=0b+1 PriorityMap=1 0b-0 2 @@ -55,4 +56,43 @@ class ConfigParseEtsPrioOptionValueTest : AbstractUnitFileTest() { assertSize(8, highlights) } + @Test + fun testBandsAboveFifteenNeverWork() { + // The parser (safe_atou8) takes 16…255, but enhanced_transmission_selection_verify() drops the section when an + // entry is not below the band count (src/network/tc/ets.c#L325-L330), which is at most 16 (#565). + // language="unit file (systemd)" + val file = """ + [EnhancedTransmissionSelection] + PriorityMap=255 + PriorityMap=0 1 16 + """.trimIndent() + + setupFileInEditor("file.network", file) + enableInspection(InvalidValueInspection::class.java) + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } + + assertSize(2, highlights) + assertStringContains("PriorityMap: band '255' is out of range. Bands are numbered 0…15", highlights[0].description) + assertStringContains("PriorityMap: band '16' is out of range.", highlights[1].description) + } + + @Test + fun testOtherBadWordsKeepTheGenericMessage() { + // Only numbers above 15 get the band message; `-1` and a negative-binary word are not "bands out of range". + // language="unit file (systemd)" + val file = """ + [EnhancedTransmissionSelection] + PriorityMap=-1 + PriorityMap=0x10 + """.trimIndent() + + setupFileInEditor("file.network", file) + enableInspection(InvalidValueInspection::class.java) + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } + + assertSize(2, highlights) + assertFalse(highlights[0].description.contains("out of range. Bands are numbered")) + assertStringContains("PriorityMap: band '0x10' is out of range.", highlights[1].description) + } + } diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseGredU32OptionValueTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseGredU32OptionValueTest.kt index 657eee48..6d044a1e 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseGredU32OptionValueTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseGredU32OptionValueTest.kt @@ -2,6 +2,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.inspections.ai import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest import net.sjrx.intellij.plugins.systemdunitfiles.inspections.InvalidValueInspection +import com.intellij.lang.annotation.HighlightSeverity import org.junit.Test class ConfigParseGredU32OptionValueTest : AbstractUnitFileTest() { @@ -17,21 +18,20 @@ class ConfigParseGredU32OptionValueTest : AbstractUnitFileTest() { DefaultVirtualQueue=12 VirtualQueues=10 DefaultVirtualQueue=10 - VirtualQueues=0 + VirtualQueues=1 DefaultVirtualQueue=0 VirtualQueues=16 - DefaultVirtualQueue=16 + DefaultVirtualQueue=15 VirtualQueues=0x10 - DefaultVirtualQueue=0x10 + DefaultVirtualQueue=0xf VirtualQueues=0b1 DefaultVirtualQueue=0b1 VirtualQueues=020 - DefaultVirtualQueue=020 + DefaultVirtualQueue=017 VirtualQueues=+3 DefaultVirtualQueue=+3 VirtualQueues=0b+1 DefaultVirtualQueue=0b+1 - VirtualQueues=0b-0 DefaultVirtualQueue=0b-0 VirtualQueues=0b 1 DefaultVirtualQueue=0b 1 @@ -39,7 +39,6 @@ class ConfigParseGredU32OptionValueTest : AbstractUnitFileTest() { DefaultVirtualQueue=0o 7 VirtualQueues=0b +1 DefaultVirtualQueue=0b +1 - VirtualQueues=0b -0 DefaultVirtualQueue=0b -0 VirtualQueues= DefaultVirtualQueue= @@ -85,4 +84,25 @@ class ConfigParseGredU32OptionValueTest : AbstractUnitFileTest() { assertSize(18, highlights) } + @Test + fun testValuesThatAlwaysFailVerify() { + // The parser takes 0…16 for both keys, but generic_random_early_detection_verify() drops the section unless + // DefaultVirtualQueue= < VirtualQueues= (src/network/tc/gred.c#L56-L65), so these can never work (#565). + // language="unit file (systemd)" + val file = """ + [GenericRandomEarlyDetection] + VirtualQueues=0 + VirtualQueues=0b-0 + DefaultVirtualQueue=16 + DefaultVirtualQueue=0x10 + """.trimIndent() + + setupFileInEditor("file.network", file) + enableInspection(InvalidValueInspection::class.java) + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } + + assertSize(4, highlights) + assertStringContains("VirtualQueues must be 1…16.", highlights[0].description) + assertStringContains("DefaultVirtualQueue must be 0…15 and below VirtualQueues=", highlights[2].description) + } } diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseRestrictFilesystemsOptionValueTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseRestrictFilesystemsOptionValueTest.kt index d7d7f5af..c410103b 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseRestrictFilesystemsOptionValueTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseRestrictFilesystemsOptionValueTest.kt @@ -2,6 +2,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.inspections.ai import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest import net.sjrx.intellij.plugins.systemdunitfiles.inspections.InvalidValueInspection +import com.intellij.lang.annotation.HighlightSeverity import org.junit.Test class ConfigParseRestrictFilesystemsOptionValueTest : AbstractUnitFileTest() { @@ -59,14 +60,30 @@ class ConfigParseRestrictFilesystemsOptionValueTest : AbstractUnitFileTest() { } @Test - fun testRuntimeOnlyRejectionsAreAccepted() { - // sd-check at 0f0682b773 accepts every one of these (one value per probe file). The unit-file parser - // stores any word that does not unquote to `@group` unchecked; unknown names are only logged and - // skipped when the unit starts ("bpf-restrict-fs: Invalid filesystem name '…', ignoring.", - // src/core/bpf-restrict-fs.c#L117-L121). Issue #565 tracks a separate warning for them. The kf6 - // autotest values ("invalid", "temporary", "@auxiliary-api btrfs nfs @temporary unknown") are here. - // A `~` word after the first position is the literal name `~proc` / `~@foo`, and quotes and - // backslash escapes are removed anywhere in a word (EXTRACT_UNQUOTE). + fun testQuotedKnownNamesAreAccepted() { + // EXTRACT_UNQUOTE removes quotes and backslash escapes anywhere in a word, so these all name ext4/xfs. + // language="unit file (systemd)" + val file = """ + [Service] + RestrictFileSystems=ext"4" + RestrictFileSystems=ext4"" + RestrictFileSystems=\ext4 'x'fs + RestrictFileSystems="@basic-api" '@network' + """.trimIndent() + + setupFileInEditor("file.service", file) + enableInspection(InvalidValueInspection::class.java) + assertSize(0, myFixture.doHighlighting()) + } + + @Test + fun testUnknownNamesAreFlagged() { + // sd-check at 0f0682b773 is silent for every one of these at load time (the parser stores plain names + // unchecked), but when the unit starts the BPF code drops each unknown name with "bpf-restrict-fs: + // Invalid filesystem name '…', ignoring." (src/core/bpf-restrict-fs.c#L116-L121, called from + // src/core/exec-invoke.c#L2123), so the entry has no effect (#565). The kf6 autotest + // values ("invalid", "temporary", "@auxiliary-api btrfs nfs @temporary unknown") are here. A `~` word + // after the first position is the literal name `~proc` / `~@foo`, which no file system has. // language="unit file (systemd)" val file = """ [Service] @@ -84,17 +101,30 @@ class ConfigParseRestrictFilesystemsOptionValueTest : AbstractUnitFileTest() { RestrictFileSystems=~~proc RestrictFileSystems="~@bogus" RestrictFileSystems=\~proc - RestrictFileSystems=ext"4" - RestrictFileSystems=ext4"" RestrictFileSystems="" RestrictFileSystems="a\"b" RestrictFileSystems='a\'b' - RestrictFileSystems="@basic-api" '@network' """.trimIndent() setupFileInEditor("file.service", file) enableInspection(InvalidValueInspection::class.java) - assertSize(0, myFixture.doHighlighting()) + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } + assertSize(17, highlights) + assertStringContains("unknown file system or group 'ext5'; systemd ignores this entry, so it neither allows nor denies anything", + highlights.single { it.text == "ext5" }.description) + assertStringContains("'~proc' is not a file system. '~' only means \"deny list\" as the very first character", + highlights.first { it.text == "~proc" }.description) + } + + @Test + fun testLongListOfUnknownNamesStaysOneProblem() { + // Each word has a single parse, so a long list of unknown names does not blow up the new engine (and fail open). + val words = (1..30).joinToString(" ") { "unknownfs$it" } + setupFileInEditor("file.service", "[Service]\nRestrictFileSystems=ext4 $words\n") + enableInspection(InvalidValueInspection::class.java) + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } + assertSize(1, highlights) + assertStringContains("unknown file system or group 'unknownfs", highlights[0].description) } @Test diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseSyscallFilterOptionValueTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseSyscallFilterOptionValueTest.kt index adb46fc6..b6d6b84c 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseSyscallFilterOptionValueTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/ConfigParseSyscallFilterOptionValueTest.kt @@ -2,6 +2,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.inspections.ai import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest import net.sjrx.intellij.plugins.systemdunitfiles.inspections.InvalidValueInspection +import com.intellij.lang.annotation.HighlightSeverity import org.junit.Test class ConfigParseSyscallFilterOptionValueTest : AbstractUnitFileTest() { @@ -100,7 +101,7 @@ class ConfigParseSyscallFilterOptionValueTest : AbstractUnitFileTest() { // language="unit file (systemd)" val nspawn = """ [Exec] - SystemCallFilter=@system-service ~@mount + SystemCallFilter=@system-service add_key SystemCallFilter=~@clock add_key """.trimIndent() setupFileInEditor("file.nspawn", nspawn) @@ -108,11 +109,12 @@ class ConfigParseSyscallFilterOptionValueTest : AbstractUnitFileTest() { } @Test - fun testTildeWordAfterTheFirstPositionIsAccepted() { + fun testTildeWordAfterTheFirstPositionWarnsInNspawn() { // nspawn only honours `~` as the first character, so "~@mount" below is stored verbatim as a name. - // sd-check at 0f0682b773 accepts all of these in a .nspawn file (nspawn's parser stores words - // unchecked); the word is only dropped at container start by seccomp_add_syscall_filter_item(), a - // runtime-only rejection that issue #565 will warn about. + // sd-check at 0f0682b773 accepts all of these in a .nspawn file (nspawn's parser stores words unchecked), + // but at container start seccomp_add_syscall_filter_item() cannot resolve the name and drops it with only a + // debug message (src/shared/seccomp-util.c#L1069-L1075), so the entry silently does nothing: a weak + // warning, not an error (#565). // language="unit file (systemd)" val nspawn = """ [Exec] @@ -123,12 +125,16 @@ class ConfigParseSyscallFilterOptionValueTest : AbstractUnitFileTest() { """.trimIndent() setupFileInEditor("file.nspawn", nspawn) enableInspection(InvalidValueInspection::class.java) - assertSize(0, myFixture.doHighlighting()) + val highlights = myFixture.doHighlighting() + assertSize(0, highlights.filter { it.severity == HighlightSeverity.WARNING }) + val warnings = highlights.filter { it.severity == HighlightSeverity.WEAK_WARNING } + assertEquals(listOf("~@mount", "~@bogus", "~@mount", "~read"), warnings.map { it.text }) + assertStringContains("SystemCallFilter: '~@mount' is ignored by systemd-nspawn. '~' is only honoured as the first character", + warnings[0].description) // In a unit file sd-check says "System call ~@mount is not known, ignoring." for the same words: the // libseccomp lookup warning that this grammar already leaves unflagged for names like "madeupcall". - // Both files share the (config_parse_syscall_filter, 0) registration, so the lenient nspawn - // behaviour applies here as well. Documented leniency. + // Unit files keep that behaviour; the nspawn warning is chosen by file type. // language="unit file (systemd)" val service = """ [Service] diff --git a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/Issue568NetworkValidatorsTest.kt b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/Issue568NetworkValidatorsTest.kt index 41ee68f8..ce447241 100644 --- a/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/Issue568NetworkValidatorsTest.kt +++ b/src/test/kotlin/net/sjrx/intellij/plugins/systemdunitfiles/inspections/ai/Issue568NetworkValidatorsTest.kt @@ -2,6 +2,7 @@ package net.sjrx.intellij.plugins.systemdunitfiles.inspections.ai import net.sjrx.intellij.plugins.systemdunitfiles.AbstractUnitFileTest import net.sjrx.intellij.plugins.systemdunitfiles.inspections.InvalidValueInspection +import com.intellij.lang.annotation.HighlightSeverity import org.junit.Test /** @@ -10,8 +11,8 @@ import org.junit.Test * file: the "valid" ones produce no complaint (or, for Address=, only the "specified without prefix length" * advice), the "invalid" ones are refused by the parser. * - * Accepted on purpose although systemd refuses them later: `[Xfrm] InterfaceId=0` (netdev verify: "Xfrm - * interface ID cannot be zero"). Validators flag only what the config parser rejects (#565 covers the rest). + * `[Xfrm] InterfaceId=0` passes the parser but is refused by xfrm_verify() ("Xfrm interface ID cannot be zero", + * src/network/netdev/xfrm.c#L30-L39), which drops the netdev, so it is flagged too (#565). */ class Issue568NetworkValidatorsTest : AbstractUnitFileTest() { @@ -62,7 +63,7 @@ class Issue568NetworkValidatorsTest : AbstractUnitFileTest() { [Xfrm] InterfaceId=0x98 InterfaceId=0x99 - InterfaceId=0 + InterfaceId=1 InterfaceId=4294967295 InterfaceId=0xffffffff InterfaceId=077 @@ -85,6 +86,33 @@ class Issue568NetworkValidatorsTest : AbstractUnitFileTest() { assertHighlights("file.netdev", file, 5) } + @Test + fun testXfrmInterfaceIdZeroIsRefusedByVerify() { + // xfrm_verify() refuses 0 and netdev_load_one() then drops the netdev (src/network/netdev/netdev.c#L1110-L1114). + // language="unit file (systemd)" + val file = """ + [Xfrm] + InterfaceId=0 + InterfaceId=0x0 + """.trimIndent() + setupFileInEditor("file.netdev", file) + enableInspection(InvalidValueInspection::class.java) + val highlights = myFixture.doHighlighting().filter { it.severity == HighlightSeverity.WARNING } + assertSize(2, highlights) + assertStringContains("InterfaceId must be 1…4294967295; systemd-networkd refuses 0", highlights[0].description) + } + + @Test + fun testUint32ZeroStillValidForOtherKeys() { + // Only [Xfrm] InterfaceId= in a .netdev has the extra rule; other config_parse_uint32 keys keep 0. + // language="unit file (systemd)" + val file = """ + [DHCPServer] + PoolOffset=0 + """.trimIndent() + assertHighlights("file.network", file, 0) + } + @Test fun testFooOverUdpProtocolValid() { // language="unit file (systemd)"