Skip to content

sim_copyfile ignored overwrite_existing parameter on POSIX hosts #541

@pmetzger

Description

@pmetzger

Found by Codex during the course of creating 100% test coverage infrastructure for my fork of SIMH. I've verified the bug is real.

Summary

sim_copyfile(const char *source_file, const char *dest_file, t_bool overwrite_existing) did not honor the overwrite_existing
parameter on the POSIX implementation in src/runtime/sim_fio.c.

On Windows, the corresponding implementation already passed the correct policy through to CopyFileA, so this was a POSIX-only behavior bug.

Broken behavior

Before the fix, the POSIX path always opened the destination with
"wb":

fOut = sim_fopen (dest_file, "wb");

That truncates and replaces an existing destination unconditionally. As a result, this call:

sim_copyfile(source, dest, FALSE)

would still overwrite dest if it already existed.

Why this is a real bug

  • the function API explicitly carries an overwrite_existing flag
  • the Windows implementation already respects that flag
  • the POSIX implementation silently ignored it

So the two host implementations disagreed on a user-visible behavior, and the POSIX version could destroy existing output files even when the caller explicitly requested that it not do so.

Fix

The POSIX implementation now uses exclusive-create open mode on modern BSD/Linux/macOS hosts:

fOut = sim_fopen (dest_file, overwrite_existing ? "wb" : "xb");

That makes the no-overwrite path atomic on those systems and aligns the behavior with the documented contract.

For any remaining non-Windows platforms that might not yet use exclusive create mode, the fallback is still to pre-check for an existing destination before opening it for write. In theory configuration before compilation could do this better, and I'll probably add such a thing eventually in my fork.

Minimal patch

+#if defined (__linux) || defined (__linux__) || defined (__APPLE__) || \
+    defined (__FreeBSD__) || defined (__NetBSD__) || defined (__OpenBSD__)
+fOut = sim_fopen (dest_file, overwrite_existing ? "wb" : "xb");
+#else
+if (!overwrite_existing && (sim_stat (dest_file, &statb) == 0)) {
+    st = sim_messagef (SCPE_ARG, "Can't open '%s' for output: %s\n",
+                       dest_file, strerror (EEXIST));
+    goto Cleanup_Return;
+    }
+fOut = sim_fopen (dest_file, "wb");
+#endif

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions