Fix Issue 18024 - checkedint.Warn should be @safe

[wilzbach]

Same pattern as in

phobos/std/stdio.d

Lines 3575 to 3578 in 5964600

	private @property File trustedStdout() @trusted
	{
	return stdout;
	}

Maybe instead of the trustedStdout workaround makeGlobal should have been made @safe?

phobos/std/stdio.d

Line 4612 in 5964600

@property ref File makeGlobal(StdFileHandle _iob)()

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18024	enhancement	checkedint.Abort should be @safe

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#5928"

[JackStouffer]

File is unsafe for a reason. Checked!Warn and Checked!Abort are currently thread unsafe and this will mark them as good to go.

I'm of the opinion that trustedStdout was a huge mistake we're still paying for.

[wilzbach]

This is a private property and only used for printing to stdout within Warn.

[wilzbach]

So can we all agree on a method that writes to stdout should be @safe
(especially when you don't even pass-in stdout)

[JackStouffer]

@wilzbach

stderr is fundamentally unsafe

import std.algorithm;
import std.experimental.checkedint;
import std.parallelism;
import std.range;
import std.stdio;

void main()
{
    stderr = File("/dev/null", "w");
    
    foreach(t; 100_000.iota.map!(a => checked!(Warn)(a)).parallel)
    {
        auto a = (t * 10000 * 1000 * 1000).get;
    }
}

$ dmd -run test.d
Error: program killed by signal 11

I highly recommend that we go with my suggestion in the bug report

IMO, Abort should be modified to not use Warn, and a new Hook called debug should give the same behavior as Abort has now. That way, by default checked is @nogc @safe nothrow pure

[JackStouffer]

Ping

[wilzbach]

I highly recommend that we go with my suggestion in the bug report

OK. Finally found time to rebase this and change to this.

That way, by default checked is @nogc @safe nothrow pure

Ehm assert still does checks for its lazy message parameter though it could be easily argued that it shouldn't as or throwing exceptions or doing GC allocation before the program terminates isn't important.

Anyhow do you have a better idea for quick @nogc string allocation than hacking it with assumeNogc?
(Abort isn't @nogc either at the moment, so this shouldn't block this)

[n8sh]

Anyhow do you have a better idea for quick @nogc string allocation than hacking it with assumeNogc?

• malloc or core.memory.pureMalloc (not generally applicable when the reason for @nogc is performance, but because the program will terminate it's okay)
• having a static or __gshared block of memory that exists only to hold such messages (in situations like this one where at compile time you have tight bounds on the size of buffer needed, and you further know that no references to the string will be retained, in this case because the program is terminating)
• don't actually allocate a string to hold your entire error message, just write the message to stderr with printf then call abort (possible in cases like this but not always)

[wilzbach]

Thanks a lot for your ideas, but I still think that if @nogc is really wanted for this, casing to @nogc with assumeNogc is probably the quickest and easiest way.

Or you know Exception's toString could support ranges or writer-based toString.

---

malloc or core.memory.pureMalloc (not generally applicable when the reason for @nogc is performance, but because the program will terminate it's okay)

Week, ideally there will be even RCString one day...
Also it's a bit more work to use format with a chunk of memory, than just a one-liner like it's at the moment.

having a static or __gshared block of memory that exists only to hold such messages (in situations like this one where at compile time you have tight bounds on the size of buffer needed, and you further know that no references to the string will be retained, in this case because the program is terminating)

That's a good idea, but wouldn't this lead to a slight overhead in startup and memory consumption for everyone using checked?

don't actually allocate a string to hold your entire error message, just write the message to stderr with printf then call abort (possible in cases like this but not always)

It's possible to catch AssertErrors even though it's officially undefined, it works quite well and is needed for e.g. Vibe.d when you don't want a single fiber crash to crash your entire web server.

[JackStouffer]

Another option is to push for https://issues.dlang.org/show_bug.cgi?id=15100

[JackStouffer]

needs an annotation

[JackStouffer]

Looks good. Just needs a changelog entry detailing the change/breakage.

[wilzbach]

Looks good. Just needs a changelog entry detailing the change/breakage.

Added.

[JackStouffer]

Ping @andralex

[andralex]

One really nice thing about the default Checked/checked was that it was issuing an explanatory message before terminating the program. That was a massive improvement over "Bus error", "Division by zero" and similar messages with little additional detail provided by lower-level facilities.

In release mode, assert(0, msg) does not print msg and provides no explanation. We could argue that this behavior should be changed (I think it should); we worry about things like the size of messages and code way too much for this era. Or we could look for a trusted means to write to stderr before aborting. Whatever we do, we should keep the nice behavior.

[andralex]

A bit of rationale would be useful

[andralex]

Multi-word phrases in backquotes are dangerous. Some editors support reflowing text. When that feature is used during document editing, the backquotes may be split across different lines, leading to wrong formatting. I use backquotes only for single-word code snippets.

[n8sh]

stderr is fundamentally unsafe

@JackStouffer Can you explain further? I tried running your example at various optimization levels with DMD and LDC and couldn't get it to segfault, although I was able to inconsistently get object.Exception@std/stdio.d(2836): Attempting to write to closed File.

[JackStouffer]

https://run.dlang.io/is/QAzYqy

To make a long story short, the ref counting causes a race condition. I'm not able to get it to happen master either, but the code hasn't changed so I'm not convinced it's fixed.

[wilzbach]

In release mode, assert(0, msg) does not print msg and provides no explanation. We could argue that this behavior should be changed (I think it should); we worry about things like the size of messages and code way too much for this era.

👍 (assert should/could be so much better)

Or we could look for a trusted means to write to stderr before aborting. Whatever we do, we should keep the nice behavior.

Yeah that's what I initially did.
I submitted my initial version as separate PR:

#6369

(though it now stops the world to be able to claim being allowed to write to stderr safely which I'm still not sure whether whether such big hacks are really necessary)

[andralex]

The thrust should go into fixing File. Basing a design on a bug elsewhere seems odd. Most likely we need to interlock the reference count operations on File. @wilzbach can you please look into that?

[JackStouffer]

The problem that came up with locking is that it was seen as a performance penalty to those using it in a non-threaded environment. The obvious answer to that is only use mutexes when it’s shared. But, as we’ve already seen, shared is not viable for file.

If you’re willing to live with the performance penalty I would love to see this happen

[n8sh]

The problem that came up with locking is that it was seen as a performance penalty to those using it in a non-threaded environment.

Maybe we could get away with just atomic refcounting, but that's still a penalty.

The obvious answer to that is only use mutexes when it’s shared. But, as we’ve already seen, shared is not viable for file.

Then maybe two different structs, File and SharedFile.

Although one might ask what we're gaining from stdout and stderr being refcounted in the first place.

[JackStouffer]

Then maybe two different structs, File and SharedFile.

Yes and that came up as well. But again, shared is not really viable for File because it requires backwards incompatible changes, and therefore cannot be used for stdout and friends.

Although one might ask what we're gaining from stdout and stderr being refcounted in the first place

There are reasons, but it's irrelevant now that we're stuck with it.

[n8sh]

If changing the type of stdout/stderr is a non-starter how about thread-locals?

[JackStouffer]

Also backwards incompatible IIRC.

The best option here is to add locks or atomic ops in File

[andralex]

We should be able to cope with the inefficiency in copying. Nevertheless, what are the incompatibilities?

[JackStouffer]

Anything which relies on the fact that stdout/stderr/stdin are globals will no longer work. E.g. a program which knows they are global and uses the lock/unlock functions for writing across threads as a pseudo mutex will no longer work. If you don't reassign stdout/stderr to some other File instance this would work just fine in the current code.

Also, you have to ask how making them thread locals would work, as core.stdc.stdio.stdin/out/err are thread global from the C definition. Therefore, a truly thread local stdin would have to reimplement stdin's FILE* and not use the C version. I remember Walter telling me that when the original code for std.file was written shared was far to buggy to be used, so he made them __gshared.

So to reiterate, we're really only left with two practical options

1. Write a shared(File) and live with the backwards incompatibility for the sake of code correctness and no performance loss for users of File
2. Add locks and/or atomic ops to File and live with the performance penalty.

[n8sh]

Also, you have to ask how making them thread locals would work

Right now they're lazily initialized __gshareds from the values of C's stdin/stdout/stderr and don't subsequently track reassignment of C's stdin/stdout/stderr. As thread-locals they could still be implemented like that.

EDIT
You'd of course need to artificially increase the refcount by 1 on all of them. You don't ever want any of them to close the stream.

[n8sh]

E.g. a program which knows they are global and uses the lock/unlock functions for writing across threads as a pseudo mutex will no longer work.

File.lock would still work since that's an OS-specific call based on the file descriptor. If you were thinking of locking/unlocking an object monitor, File is a struct so it doesn't have that.

[JackStouffer]

and don't subsequently track reassignment of C's stdin/stdout/stderr

Not sure what you mean here because you can't reassign C's stdout.

[andralex]

@JackStouffer I guess freopen does that.

[n8sh]

@JackStouffer That appears to be platform-dependent.

https://www.gnu.org/software/libc/manual/html_node/Standard-Streams.html

In the GNU C Library, stdin, stdout, and stderr are normal variables which you can set just like any others. For example, to redirect the standard output to a file, you could do:

fclose (stdout);
stdout = fopen ("standard-output-file", "w");

Note however, that in other systems stdin, stdout, and stderr are macros that you cannot assign to in the normal way.

[andralex]

Add locks and/or atomic ops to File and live with the performance penalty.

This should be fine.

[JackStouffer]

That appears to be platform-dependent.

Silly me, I assumed the implementation of standard output be standardized.

[JackStouffer]

#6382

[JackStouffer]

Now, after #6382 is merged, I still think there is still something blocking this: the issue of the unsafe reassignment of stdout/in/err.

The most viable solution as I see to the problem of possible data races in stderr reassignment is to modify stderr/stdout to both be two @property functions, one which returns the global File instance and one which accepts a file instance and modifies the gshared data. Both would lock using core.internal.spinlock. This would be a small breaking change because reassignment would no longer be pure. The breakage would be minimal because File itself isn't pure; still, breakage is possible.

It would then be prudent to mark these property functions as @trusted and then to mark trustedStdout as deprecated. This would also solve the problem of writeln being @safe and stdout.writeln being @system.

Thoughts?

[n8sh]

This would be a small breaking change because reassignment would no longer be pure

How could reassigning a global variable possibly be pure?

[JackStouffer]

How could reassigning a global variable possibly be pure?

It wouldn't, silly mistake.

[wilzbach]

See also #6550 for yet another PR to make checkedint @safe

[RazvanN7]

This has been fixed by: #7909