Skip to content

auto-fix bin/sbin duplicates #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lnykryn wants to merge 2 commits into
base: main
Choose a base branch
from
+151 −7
Open

auto-fix bin/sbin duplicates #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4eab6f
bump fedora used in mkosi to rawhide
lnykryn Nov 7, 2025
8d6d448
alternatives: refactor state management and auto-fix bin/sbin duplicates
lnykryn Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 150 additions & 6 deletions alternatives.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,84 @@ void nextLine(char **buf, char **line) {
*line = parseLine(buf);
}

static int readConfig(struct alternativeSet *set, const char *title,
const char *altDir, const char *stateDir, int flags) {
/* Forward declaration */
static int writeStateConf(struct alternativeSet *set, const char *stateDir, int flags);

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

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
*/
static int removeBinSbinDuplicates(struct alternativeSet *set, int flags) {
int i, j;
int removedCount = 0;

if (!MERGED_SBIN || set->numAlts < 2)
return 0;

for (i = 0; i < set->numAlts; i++) {
for (j = i + 1; j < set->numAlts; j++) {
/* Check if they differ only in bin/sbin (or are exactly the same) */
if (streq_bin(set->alts[i].leader.target, set->alts[j].leader.target)) {
int k;

/* 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;
}
Comment on lines +360 to +365
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
/* If any follower didn't match, skip this pair */
if (k < set->alts[i].numFollowers)
continue;

/* Keep i if it is not in /sbin/, otherwise keep j */
if (strstr(set->alts[i].leader.target, "/sbin/")) {
if (FL_VERBOSE(flags)) {
fprintf(stderr, _("removing duplicate alternative: %s (keeping %s)\n"),
set->alts[i].leader.target, set->alts[j].leader.target);
}
clearAlternative(&set->alts[i]);
set->alts[i] = set->alts[j];
if (set->best == j)
set->best = i;
if (set->current == j)
set->current = i;
} 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];
Comment on lines +391 to +392
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[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 uses AI. Check for mistakes.
}
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--;
Comment on lines +382 to +404
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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

  1. Clears set->alts[i]
  2. Copies set->alts[j] to set->alts[i]
  3. 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 uses AI. Check for mistakes.
}
}
}

return removedCount;
}

/*
* Internal function to read alternatives configuration.
* Reads the config file and verifies symlinks without any modifications.
*/
static int readConfigInternal(struct alternativeSet *set, const char *title,
const char *altDir, const char *stateDir, int flags) {
char *path;
char *leader_path;
int fd;
Expand Down Expand Up @@ -587,6 +663,47 @@ static int readConfig(struct alternativeSet *set, const char *title,
return r;
}

/*
* Read alternatives configuration and remove bin/sbin duplicates if found.
* This is the main entry point for reading config.
*/
static int readConfig(struct alternativeSet *set, const char *title,
const char *altDir, const char *stateDir, int flags) {
int rc;

rc = readConfigInternal(set, title, altDir, stateDir, flags);
if (rc)
return rc;

/* If we removed duplicates, rewrite the config file and re-read */
if (removeBinSbinDuplicates(set, flags) > 0) {
if (FL_VERBOSE(flags))
fprintf(stderr, _("rewriting config after removing duplicates\n"));

if (!FL_TEST(flags)) {
rc = writeStateConf(set, stateDir, flags);
if (rc) {
fprintf(stderr, _("failed to write cleaned config for %s\n"), title);
return rc;
}

/* Free existing data before re-reading */
for (int i = 0; i < set->numAlts; i++) {
clearAlternative(&set->alts[i]);
}
free(set->alts);
free(set->currentLink);

Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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:

  1. Memset the entire set structure to 0 before re-reading, or
  2. 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 uses AI. Check for mistakes.
/* Re-read the config to ensure consistency */
rc = readConfigInternal(set, title, altDir, stateDir, flags);
if (rc)
return rc;
}
}

return 0;
}

static int isLink(char *path) {
struct stat sbuf;

Expand Down Expand Up @@ -706,15 +823,16 @@ static int makeLinks(struct linkSet *l, const char *altDir, int flags) {
return 0;
}

static int writeState(struct alternativeSet *set, const char *altDir,
const char *stateDir, int forceLinks, int flags) {
/*
* Write the alternatives configuration file to stateDir.
* This only updates the config file, not the actual symlinks.
*/
static int writeStateConf(struct alternativeSet *set, const char *stateDir, int flags) {
char *path;
char *path2;
int fd;
FILE *f;
int i, j;
int rc = 0;
struct alternative *alt;

path = alloca(strlen(stateDir) + strlen(set->alts[0].leader.title) + 6);
sprintf(path, "%s/%s.new", stateDir, set->alts[0].leader.title);
Expand Down Expand Up @@ -770,6 +888,20 @@ static int writeState(struct alternativeSet *set, const char *altDir,
return 1;
}

return 0;
}

/*
* Update the actual symlinks and initscripts for the current alternative.
* Should be called after writeStateConf.
*/
static int writeStatePaths(struct alternativeSet *set, const char *altDir,
int forceLinks, int flags) {
char *path;
int i;
int rc = 0;
struct alternative *alt;

if (set->mode == AUTO)
set->current = set->best;

Expand Down Expand Up @@ -828,6 +960,18 @@ static int writeState(struct alternativeSet *set, const char *altDir,
return rc;
}

static int writeState(struct alternativeSet *set, const char *altDir,
const char *stateDir, int forceLinks, int flags) {
int rc;

rc = writeStateConf(set, stateDir, flags);
if (rc)
return rc;

rc = writeStatePaths(set, altDir, forceLinks, flags);
return rc;
}

static int linkCmp(const void *a, const void *b) {
struct linkSet *one = (struct linkSet *)a, *two = (struct linkSet *)b;

Expand Down
2 changes: 1 addition & 1 deletion mkosi.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[Distribution]
Distribution=fedora
Release=41
Release=rawhide

[Build]
BuildDirectory=build/mkosi.builddir
Expand Down