gh-126606: don't write incomplete pyc files

[cfbolz]

fix corner case in importlib: so far, the code does not check the return value of _io.FileIO.write to see whether a pyc code was fully written to disk. If a ulimit is in place (or the disk is full or something), this could lead to truncated pyc files. After the fix, the written size is checked and the half-written file is removed.

• Issue: Corrupt .pyc files stay on disk after failed writes #126606

[cmaloney]

The write can be partial for a number of reasons, and while some of those are retried (PEP-0475), things like "this is windows and the write size is capped" aren't retried. Could this retry, which hopefully should also mean the OSError contains underlying errno / error info?

FileIO.write calls os.write calls _Py_write calls _Py_write_impl:

cpython/Python/fileutils.c

Lines 1918 to 2016 in 6293d00

	_Py_write_impl(int fd, const void *buf, size_t count, int gil_held)
	{
	Py_ssize_t n;
	int err;
	int async_err = 0;
	_Py_BEGIN_SUPPRESS_IPH
	#ifdef MS_WINDOWS
	if (count > 32767) {
	/* Issue #11395: the Windows console returns an error (12: not
	enough space error) on writing into stdout if stdout mode is
	binary and the length is greater than 66,000 bytes (or less,
	depending on heap usage). */
	if (gil_held) {
	Py_BEGIN_ALLOW_THREADS
	if (isatty(fd)) {
	count = 32767;
	}
	Py_END_ALLOW_THREADS
	} else {
	if (isatty(fd)) {
	count = 32767;
	}
	}
	}
	#endif
	if (count > _PY_WRITE_MAX) {
	count = _PY_WRITE_MAX;
	}
	if (gil_held) {
	do {
	Py_BEGIN_ALLOW_THREADS
	errno = 0;
	#ifdef MS_WINDOWS
	// write() on a non-blocking pipe fails with ENOSPC on Windows if
	// the pipe lacks available space for the entire buffer.
	int c = (int)count;
	do {
	_doserrno = 0;
	n = write(fd, buf, c);
	if (n >= 0 || errno != ENOSPC || _doserrno != 0) {
	break;
	}
	errno = EAGAIN;
	c /= 2;
	} while (c > 0);
	#else
	n = write(fd, buf, count);
	#endif
	/* save/restore errno because PyErr_CheckSignals()
	* and PyErr_SetFromErrno() can modify it */
	err = errno;
	Py_END_ALLOW_THREADS
	} while (n < 0 && err == EINTR &&
	!(async_err = PyErr_CheckSignals()));
	}
	else {
	do {
	errno = 0;
	#ifdef MS_WINDOWS
	// write() on a non-blocking pipe fails with ENOSPC on Windows if
	// the pipe lacks available space for the entire buffer.
	int c = (int)count;
	do {
	_doserrno = 0;
	n = write(fd, buf, c);
	if (n >= 0 || errno != ENOSPC || _doserrno != 0) {
	break;
	}
	errno = EAGAIN;
	c /= 2;
	} while (c > 0);
	#else
	n = write(fd, buf, count);
	#endif
	err = errno;
	} while (n < 0 && err == EINTR);
	}
	_Py_END_SUPPRESS_IPH
	if (async_err) {
	/* write() was interrupted by a signal (failed with EINTR)
	and the Python signal handler raised an exception (if gil_held is
	nonzero). */
	errno = err;
	assert(errno == EINTR && (!gil_held || PyErr_Occurred()));
	return -1;
	}
	if (n < 0) {
	if (gil_held)
	PyErr_SetFromErrno(PyExc_OSError);
	errno = err;
	return -1;
	}
	return n;
	}

[cfbolz]

I don't know whether we should retry here. in a sense that's a separate question to the bug that I'm trying to fix. we should definitely not silently write a truncated pyc file, as we are right now.

[cmaloney]

My thought is “why didn’t the write error itself” and doing a second one / retry might fix that

[cfbolz]

because write doesn't error, it just writes fewer bytes. from the man page:

The number of bytes written may be less than count if, for example, there is insufficient space on the underlying physical medium, or the RLIMIT_FSIZE resource limit is encountered (see setrlimit(2)), or the call
was interrupted by a signal handler after having written less than count bytes. (See also pipe(7).)

https://man7.org/linux/man-pages/man2/write.2.html#DESCRIPTION

[cmaloney]

Also from that though, a second call won't return 0, either it will write more and rerturn the count, or it will error (which should make the OSError):

In the event of a partial write, the caller can make another write() call to transfer the remaining bytes. The subsequent call will either transfer further bytes or may result in an error (e.g., if the disk is now full).

[brettcannon]

Considering this issue hasn't been reported until now I don't think it's worth complicating the code to try and be clever to work around some odd OS restriction.

[brettcannon]

Some stylistic comments on the test and a question of what the best exception is.

[brettcannon]

Considering this issue hasn't been reported until now I don't think it's worth complicating the code to try and be clever to work around some odd OS restriction.

[brettcannon]

Suggested change

	raise OSError("os.write didn't write the full pyc file")
	raise OSError("os.write() didn't write the full .pyc file")

[brettcannon]

I'm trying to decide if OSError is the best exception or ImportError? While it does seem like the OS is the reason the write didn't fully succeed, it's import deciding that writing less isn't acceptable.

[cmaloney]

OSError is what surrounding code expects / the except after this catches to try and remove the partially-written file.

[brettcannon]

Suggested change

	# emulate an os.write that only writes partial data
	# Emulate an os.write that only writes partial data.

[brettcannon]

Suggested change

	# need to patch _io to be _pyio, so that io.FileIO is affected by the
	# os.write patch
	# Need to patch _io to be _pyio, so that io.FileIO is affected by the
	# os.write patch.

[brettcannon]

Why the local imports?

[brettcannon]

Suggested change

	os.stat(os_helper.TESTFN) # did not get written
	os.stat(os_helper.TESTFN) # Check the file d id not get written.

[brettcannon]

You don't have to go as far as mocking if you're just swapping out an attribute:

cpython/Lib/test/support/__init__.py

Lines 1363 to 1392 in 03924b5

	@contextlib.contextmanager
	def swap_attr(obj, attr, new_val):
	"""Temporary swap out an attribute with a new object.
	Usage:
	with swap_attr(obj, "attr", 5):
	...
	This will set obj.attr to 5 for the duration of the with: block,
	restoring the old value at the end of the block. If `attr` doesn't
	exist on `obj`, it will be created and then deleted at the end of the
	block.
	The old value (or None if it doesn't exist) will be assigned to the
	target of the "as" clause, if there is one.
	"""
	if hasattr(obj, attr):
	real_val = getattr(obj, attr)
	setattr(obj, attr, new_val)
	try:
	yield real_val
	finally:
	setattr(obj, attr, real_val)
	else:
	setattr(obj, attr, new_val)
	try:
	yield
	finally:
	if hasattr(obj, attr):
	delattr(obj, attr)

[brettcannon]

Since this is a magic constant for the test, it might be best to put it in a variable and then use that variable below to guarantee the amount of bytes written is smaller than the number of bytes sent in to write.

[bedevere-app]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.