auto-fix bin/sbin duplicates by lnykryn · Pull Request #159 · fedora-sysv/chkconfig

lnykryn · 2025-11-13T10:49:03Z

No description provided.

Split writeState into two functions for better separation: - writeStateConf() - only writes the config file - writeStatePaths() - only updates symlinks and initscripts - writeState() - wrapper that calls both (backward compatible) Split readConfig into two functions: - readConfigInternal() - reads raw config without modifications - readConfig() - wrapper that detects and removes bin/sbin duplicates Add automatic duplicate handling in readConfig(): - Scans for alternatives that differ only in /bin vs /sbin paths - Prefers alternatives in /bin or /usr/bin over /sbin or /usr/sbin - Automatically rewrites config file when duplicates are removed - Reports actions with --verbose flag Add forward declaration of writeStateConf for readConfig to use. This automatically fixes bin/sbin duplicate issues that occurred when directories were merged before alternatives was updated, with no user intervention required. The system normalizes to canonical /bin paths.

Copilot

Pull Request Overview

This PR implements automatic detection and removal of duplicate alternatives that differ only in bin vs sbin paths, which occurs in systems with merged /bin and /sbin directories. The implementation refactors the config reading and writing code to detect and clean up these duplicates.

Key changes:

Added removeBinSbinDuplicates() function to detect and remove bin/sbin duplicate alternatives
Refactored readConfig() to automatically clean up duplicates during config read
Split writeState() into writeStateConf() and writeStatePaths() for granular control
Updated mkosi.conf to use Fedora rawhide for testing

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
mkosi.conf	Updated Fedora release from 41 to rawhide for testing with latest merged filesystem changes
alternatives.c	Added duplicate detection/removal logic, refactored config read/write functions to support automatic cleanup of bin/sbin duplicates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-13T10:57:12Z

alternatives.c

+                /* Check if all followers match */
+                for (k = 0; k < set->alts[i].numFollowers; k++) {
+                    if (!streq_bin(set->alts[i].followers[k].target,
+                                  set->alts[j].followers[k].target))
+                        break;
+                }


This code assumes both alternatives have the same number of followers without checking. If set->alts[j].numFollowers differs from set->alts[i].numFollowers, this will either miss followers or access memory out of bounds.

Add a check before the loop:

/* Check if they have the same number of followers */ if (set->alts[i].numFollowers != set->alts[j].numFollowers) continue; /* Check if all followers match */ for (k = 0; k < set->alts[i].numFollowers; k++) {

Copilot · 2025-11-13T10:57:13Z

alternatives.c

+                } else {
+                    if (FL_VERBOSE(flags)) {
+                        fprintf(stderr, _("removing duplicate alternative: %s (keeping %s)\n"),
+                                set->alts[j].leader.target, set->alts[i].leader.target);
+                    }
+                    clearAlternative(&set->alts[j]);
+                }
+
+                /* Always remove j (shift everything down) */
+                for (int k = j; k < set->numAlts - 1; k++) {
+                    set->alts[k] = set->alts[k + 1];
+                }
+                set->numAlts--;
+                removedCount++;
+
+                /* Adjust current and best indices if they're after j */
+                if (set->current > j)
+                    set->current--;
+                if (set->best > j)
+                    set->best--;
+
+                /* Adjust j since we removed an element */
+                j--;


There's a logic error in this branch. When set->alts[i] is in /sbin/, the code:

Clears set->alts[i]

Copies set->alts[j] to set->alts[i]

Then at line 391-393, shifts all elements from j down, overwriting set->alts[j]

This means set->alts[i] now contains a shallow copy of data that was just shifted/moved. Since clearAlternative was already called on the original set->alts[i], and set->alts[j] is being moved (not copied), this creates a double-free risk.

The fix is to NOT shift when keeping j's value in i. The removal should only happen in the else branch. The code should be restructured to:

Identify which alternative to remove (either i or j)

Clear that alternative

If removing i, move j to i's position

Shift all elements after the removed index down

Suggested change

} else {

if (FL_VERBOSE(flags)) {

fprintf(stderr, _("removing duplicate alternative: %s (keeping %s)\n"),

set->alts[j].leader.target, set->alts[i].leader.target);

}

clearAlternative(&set->alts[j]);

}

/* Always remove j (shift everything down) */

for (int k = j; k < set->numAlts - 1; k++) {

set->alts[k] = set->alts[k + 1];

}

set->numAlts--;

removedCount++;

/* Adjust current and best indices if they're after j */

if (set->current > j)

set->current--;

if (set->best > j)

set->best--;

/* Adjust j since we removed an element */

j--;

/* Now remove j (which is now a duplicate of i) */

for (int k = j; k < set->numAlts - 1; k++) {

set->alts[k] = set->alts[k + 1];

}

set->numAlts--;

removedCount++;

/* Adjust current and best indices if they're after j */

if (set->current > j)

set->current--;

if (set->best > j)

set->best--;

/* Adjust j since we removed an element */

j--;

} else {

if (FL_VERBOSE(flags)) {

fprintf(stderr, _("removing duplicate alternative: %s (keeping %s)\n"),

set->alts[j].leader.target, set->alts[i].leader.target);

}

clearAlternative(&set->alts[j]);

for (int k = j; k < set->numAlts - 1; k++) {

set->alts[k] = set->alts[k + 1];

}

set->numAlts--;

removedCount++;

/* Adjust current and best indices if they're after j */

if (set->current > j)

set->current--;

if (set->best > j)

set->best--;

/* Adjust j since we removed an element */

j--;

}

Copilot · 2025-11-13T10:57:13Z

alternatives.c

+            }
+            free(set->alts);
+            free(set->currentLink);
+


After freeing the data structures, the code re-reads the config into the same set structure. However, readConfigInternal initializes set->alts to NULL and set->numAlts to 0 (lines 436-437), but it doesn't reset other fields like set->mode, set->best, and set->current that may have been set during the first read. While readConfigInternal does set these fields (lines 438-440), there's a potential issue if the re-read fails partway through - the set would be in an inconsistent state.

Consider either:

Memset the entire set structure to 0 before re-reading, or

Move the initialization of all set fields to the very beginning of readConfigInternal

Suggested change

/* Zero out the set structure to avoid inconsistent state */

memset(set, 0, sizeof(*set));

Copilot · 2025-11-13T10:57:14Z

alternatives.c

+                for (int k = j; k < set->numAlts - 1; k++) {
+                    set->alts[k] = set->alts[k + 1];


[nitpick] Variable k is declared within the nested if statement block (line 358), but it's used in the for loop (line 391) at the same nesting level. While this is valid C, the variable k in line 391 shadows the earlier declaration from line 358. This could be confusing.

Consider using a different variable name for the shift loop, such as idx or m, to avoid shadowing:

for (int m = j; m < set->numAlts - 1; m++) { set->alts[m] = set->alts[m + 1]; }

Suggested change

for (int k = j; k < set->numAlts - 1; k++) {

set->alts[k] = set->alts[k + 1];

for (int m = j; m < set->numAlts - 1; m++) {

set->alts[m] = set->alts[m + 1];

Copilot · 2025-11-13T10:57:14Z

alternatives.c

+
+/*
+ * Detect and remove bin/sbin duplicate alternatives.
+ * Returns the number of duplicates removed.


The function comment states "Returns the number of duplicates removed" but doesn't document important behavior:

The function modifies the alternativeSet in place

It may adjust the best and current indices

It prefers keeping alternatives with paths NOT containing /sbin/

Only operates when MERGED_SBIN is enabled

Consider expanding the documentation:

/* * Detect and remove bin/sbin duplicate alternatives. * * When MERGED_SBIN is enabled, this function identifies alternatives that differ * only in bin vs sbin paths and removes duplicates, preferring to keep alternatives * with /bin/ paths over /sbin/ paths. * * The function modifies the set in place, removing duplicate entries and adjusting * the best and current indices as needed. * * Returns: The number of duplicates removed, or 0 if MERGED_SBIN is not enabled. */

Suggested change

* Returns the number of duplicates removed.

*

* When MERGED_SBIN is enabled, this function identifies alternatives that differ

* only in bin vs sbin paths and removes duplicates, preferring to keep alternatives

* with /bin/ paths over /sbin/ paths.

*

* The function modifies the set in place, removing duplicate entries and adjusting

* the best and current indices as needed.

*

* Returns: The number of duplicates removed, or 0 if MERGED_SBIN is not enabled.

lnykryn added 2 commits November 7, 2025 14:43

bump fedora used in mkosi to rawhide

f4eab6f

lnykryn requested a review from Copilot November 13, 2025 10:52

Copilot started reviewing on behalf of lnykryn November 13, 2025 10:53 View session

Copilot finished reviewing on behalf of lnykryn November 13, 2025 10:56

Copilot AI reviewed Nov 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto-fix bin/sbin duplicates#159

auto-fix bin/sbin duplicates#159
lnykryn wants to merge 2 commits intofedora-sysv:mainfrom
lnykryn:dedup

lnykryn commented Nov 13, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



	/* Zero out the set structure to avoid inconsistent state */
	memset(set, 0, sizeof(*set));

		for (int k = j; k < set->numAlts - 1; k++) {
		set->alts[k] = set->alts[k + 1];

- * Returns the number of duplicates removed.
+ *
+ * When MERGED_SBIN is enabled, this function identifies alternatives that differ
+ * only in bin vs sbin paths and removes duplicates, preferring to keep alternatives
+ * with /bin/ paths over /sbin/ paths.
+ *
+ * The function modifies the set in place, removing duplicate entries and adjusting
+ * the best and current indices as needed.
+ *
+ * Returns: The number of duplicates removed, or 0 if MERGED_SBIN is not enabled.

Conversation

lnykryn commented Nov 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants