Make std.stdio.File.readln @safe #8623

ntrel · 2022-11-14T15:39:07Z

Make readlnImpl @trusted.
Make File.readln overloads @safe.

dlang-bot · 2022-11-14T15:39:10Z

Thanks for your pull request and interest in making D better, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub run digger -- build "master + phobos#8623"

ntrel · 2022-11-14T18:24:25Z

Unrelated ae failure:
std.socket.SocketOSException@std/socket.d(2826): Unable to bind socket: Cannot assign requested address

RazvanN7 · 2022-11-15T14:46:23Z

I'm a bit reluctant to trust such a large function. Are there many operations that prevent it from being @safe?

This reverts commit a5912ca.

ntrel · 2022-11-16T10:19:03Z

@RazvanN7 Only 34 ;-) I have made readlnImpl @safe now using @trusted lambdas.

dkorpel · 2022-11-16T11:12:18Z

std/stdio.d

 {
+    alias trusted_FLOCK = (fps) @trusted => _FLOCK(fps);


This is a controversial pattern, because even though it's inside a function, it doesn't have a @safe interface and can cause memory corruption when called wrongly inside this @safe function body. (@aG0aep6G)

Without refactoring, the correct thing to do is mark the function @trusted. The @trusted lambda's may give the illusion that the surface of @trusted code is smaller, but you could still escape FILE* fps or add a wrong FLOCK/FUNLOCK call anywhere in the @safe body.

This is a controversial pattern, because even though it's inside a function, it doesn't have a @safe interface and can cause memory corruption when called wrongly inside this @safe function body. (@aG0aep6G)

I am pleased to see that my constant nagging is having an effect :)

One way to fix this is to encapsulate all the necessary trusted functionality into a static struct that is non-copyable, and use that instead.

Ideally that struct's data would be @system, but that's not in the language yet.

@schveiguy It works with -preview=systemVariables (dlang/dmd#14478). I have implemented this. There is still one hole though, the user can call destroy on the struct instance. That will unlock even though an unshared file pointer may still be active (though access is protected with scope, the struct is still in scope).

The destructor should null the pointer.

Why? destroy (or move) already does that.

OK, I misunderstood. I didn't realize you were exposing the pointer elsewhere.

RazvanN7 · 2022-11-16T12:05:51Z

I'm sorry if I wasn't clear, but I wasn't asking to put all unsafe operations in trusted lambdas. I was just asking how prevalent are the unsafe patterns in the idea that maybe we could get rid of them.

@safe

Instead of wrong trusted_FUNLOCK. Note: trusted_FGETC does have a @safe interface.

schveiguy · 2022-11-17T16:27:05Z

std/stdio.d

+    version (DIGITAL_MARS_STDIO)
+    {
+        auto lf = LockedFile(fps);
+        auto fp = lf.fp;


This defeats the purpose of the struct. The struct can't just be a wrapper for the type itself, it needs to be a wrapper for the functionality that should be trusted. So like instead of trusted_FGETWC, the struct should have a method on it that is trusted.

Ok, but I'm not sure how this defeats the purpose of the struct. What is actually unsafe if fp can only be accessed when the file is locked (aside from the destroy issue)?

FILE * fp; { auto lf = LockedFile(fps); fp = lf.fp; }

In other words, you should not be able to use the unlocked FILE * if the struct no longer exists.

(I plan to move trusted_FGETC to a struct method as that's better anyway).

lf.fp does not return a FILE*, it's a _iobuf*. On Windows I get:

Error: cannot implicitly convert expression `lf.fp()` of type `_iobuf*` to `shared(_iobuf)*`

But all the other code paths use a cast too so I assume they would trigger the same error. Aside from the type error, the assignment fp = lf.fp should fail due to -dip1000 but it doesn't:
https://forum.dlang.org/post/cvrzlycycprixbadbpgg@forum.dlang.org

OK, I didn't know what exact type fp was returning, this wasn't my point though, I should have been more general in the example:

typeof(LockedFile.fp) fp; {auto lf = LockedFile(fps); fp = lf.fp; }

The point is, if you just expose the pointer then it defeats the purpose of encapsulating the trusted behavior.

We can use `fps` for `ferror` calls instead.

schveiguy · 2022-11-19T17:10:14Z

std/stdio.d

+    @disable void opAssign(LockedFile);
+
+    // Since fps is now locked, we can cast away shared
+    @trusted fp() return scope => cast(_iobuf*) fps;


This method should be @system

I've removed this method now. I assume you said that because return scope doesn't enforce that the result doesn't live longer than the struct. That seems to be a significant missing piece with -dip1000.

I mean the point is to encapsulate the unlocked pointer away from usage.

I didn't realize you weren't actually storing the unlocked pointer, but using this to cast it. I was expecting the LockedFile to be storing the unlocked pointer, and protect it from incorrect usage.

schveiguy · 2022-11-19T17:55:56Z

std/stdio.d

+    {
+        auto lf = LockedFile(fps);
+        // Since fps is now locked, we can cast away shared
+        auto fp = (() @trusted => cast(_iobuf*) fps)();


this shouldn't be done at all, use the LockedFile. Just add any needed trusted methods to the struct.

If all trusted operations are encapsulated in the struct, then you don't have to worry about this function in terms of safety (you do right now, because @system data isn't yet a thing, but eventually).

Just add any needed trusted methods to the struct

Well, they're fields:

stdio.d:5509: if (__fhnd_info[fp._file] & FHND_WCHAR) stdio.d:5541: else if (fp._flag & _IONBF) stdio.d:5565: int u = fp._cnt; stdio.d:5566: char* p = fp._ptr; stdio.d:5568: if (fp._flag & _IOTRAN) stdio.d:5610: fp._cnt -= i; stdio.d:5611: () @trusted { fp._ptr += i; }();

Maybe I could add an opDispatch.

If all trusted operations are encapsulated in the struct, then you don't have to worry about this function in terms of safety

I already need to call a trusted lambda to call free. I don't really see what's wrong with immediately calling a trusted lambda, the @trustedness is not being hidden. It's interface is safe at that point in the execution.

OK, I suppose if there's a lf.destroy call added then the fp is not safe.

Most of this does not need to happen on the structure itself, use locals (it's actually faster anyway to manipulate local pointers/counters than use an indirection, from my experience in iopipe). Also, if needed you can encapsulate the whole functionality into the struct itself, or make this section a trusted lambda.

Yeah, I'm looking at the code now, and actually I think this isn't getting the picture across, I'm sorry, trusted code is just so hard to get right. The opDispatch is incorrectly marked safe, because it's treating the pointer as a safe pointer, when it should be a system pointer.

When I get a few minutes, I'll rewrite what I think it should be.

@schveiguy I think I've got it right now, thanks for your help. I put all the digital mars stdio code in a single trusted lambda and did the same for the getdelim code too.

Looks a lot better! One thing I would recommend, store the actual pointer under a more obscure name, and access via a @system accessor, which will enable proper safety checking even without the @system data DIP implemented. Currently the simple access of the private member is still considered @safe by the compiler, and this will at least involve the enforcement, even if the requirement is artificial.

e.g.:

private struct LockedFile { @system private _iobuf* _fp; // obscure name to avoid accidental usage @system _iobuf* fp() { return _fp; } // use this in all @trusted calls ... }

When the system variable DIP is default, this can be changed to just a field.

Use @trusted lambda for DIGITAL_MARS_STDIO impl

dkorpel · 2022-12-01T14:56:11Z

Is this ready to go?

ntrel · 2022-12-04T17:35:23Z

@dkorpel Yes IMO. Steven recommended a workaround until @system variables are out of preview, but is that change needed?

#8623 (comment)

dkorpel · 2022-12-05T11:13:12Z

is that change needed?

I don't think so. It's internal / private anyway, and accessing system variables by default already outputs deprecation messages.

ntrel · 2022-12-10T11:49:07Z

Does anyone know if it's safe to make stdio stdout stderr @trusted? Then free function readln can be @safe as well as File.readln.

Make std.stdio.File.readln @safe

5267ccf

ntrel marked this pull request as ready for review November 14, 2022 18:24

ntrel requested review from CyberShadow and schveiguy as code owners November 14, 2022 18:24

atilaneves approved these changes Nov 15, 2022

View reviewed changes

ntrel added 3 commits November 15, 2022 17:32

Make readlnImpl @safe and use @trusted lambdas

40df30b

Use slice instead of pointer to avoid lambdas

a5912ca

Make StdioException.opCall @safe

cf131b7

ntrel force-pushed the safe-readln branch from 4bf5c6a to cf131b7 Compare November 15, 2022 17:40

3 more @trusted calls

597e73f

ntrel force-pushed the safe-readln branch from 3d9dd66 to 597e73f Compare November 15, 2022 17:52

ntrel added 4 commits November 15, 2022 17:57

Make ReadlnAppender.putonly take const array

2da49ee

fp._ptr arithmetic

01c80ba

Revert "Use slice instead of pointer to avoid lambdas"

80be7fd

This reverts commit a5912ca.

Remove unnecessary trusted lambda

9f05dd4

dkorpel reviewed Nov 16, 2022

View reviewed changes

Use LockedFile struct to enforce safe interface

b036f74

Instead of wrong trusted_FUNLOCK. Note: trusted_FGETC does have a @safe interface.

ntrel force-pushed the safe-readln branch from 291518f to b036f74 Compare November 17, 2022 11:51

ntrel mentioned this pull request Nov 17, 2022

Error on => shortened method syntax dlang-community/D-Scanner#879

Closed

schveiguy reviewed Nov 17, 2022

View reviewed changes

ntrel added 4 commits November 19, 2022 14:39

Move trusted_FGETC to LockedFile method

b5df501

Tweak LockedFile unittest

96ac2e8

Remove 2 unnecessary vars

59749cb

Remove LockedFile.fp method

89ce0c9

We can use `fps` for `ferror` calls instead.

schveiguy reviewed Nov 19, 2022

View reviewed changes

ntrel force-pushed the safe-readln branch from 127105d to 4796637 Compare November 19, 2022 20:26

ntrel added 3 commits November 19, 2022 20:26

Add LockedFile.opDispatch

4796637

Remove unsafe opDispatch

7b8fbf0

Use @trusted lambda for DIGITAL_MARS_STDIO impl

Remove double @trusted

f060308

ntrel force-pushed the safe-readln branch from 8d4e8de to f060308 Compare November 25, 2022 10:50

Fix getdelim trusted encapsulation

6c9a746

dkorpel approved these changes Dec 5, 2022

View reviewed changes

dkorpel merged commit 09275da into dlang:master Dec 5, 2022

ntrel deleted the safe-readln branch December 5, 2022 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make std.stdio.File.readln @safe #8623

Make std.stdio.File.readln @safe #8623

ntrel commented Nov 14, 2022

dlang-bot commented Nov 14, 2022

ntrel commented Nov 14, 2022

RazvanN7 commented Nov 15, 2022

ntrel commented Nov 16, 2022

dkorpel Nov 16, 2022

aG0aep6G Nov 16, 2022

schveiguy Nov 17, 2022

schveiguy Nov 17, 2022

ntrel Nov 17, 2022 •

edited

Loading

schveiguy Nov 17, 2022

ntrel Nov 19, 2022

schveiguy Nov 19, 2022

RazvanN7 commented Nov 16, 2022

schveiguy Nov 17, 2022

ntrel Nov 17, 2022

schveiguy Nov 17, 2022

schveiguy Nov 17, 2022

ntrel Nov 19, 2022

schveiguy Nov 19, 2022 •

edited

Loading

schveiguy Nov 19, 2022

ntrel Nov 19, 2022

schveiguy Nov 19, 2022 •

edited

Loading

schveiguy Nov 19, 2022

ntrel Nov 19, 2022

ntrel Nov 19, 2022

schveiguy Nov 21, 2022

ntrel Nov 25, 2022

schveiguy Nov 25, 2022

dkorpel commented Dec 1, 2022

ntrel commented Dec 4, 2022 •

edited

Loading

dkorpel commented Dec 5, 2022

ntrel commented Dec 10, 2022

Make std.stdio.File.readln @safe #8623

Make std.stdio.File.readln @safe #8623

Conversation

ntrel commented Nov 14, 2022

dlang-bot commented Nov 14, 2022

Bugzilla references

Testing this PR locally

ntrel commented Nov 14, 2022

RazvanN7 commented Nov 15, 2022

ntrel commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntrel Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schveiguy Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schveiguy Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkorpel commented Dec 1, 2022

ntrel commented Dec 4, 2022 • edited Loading

dkorpel commented Dec 5, 2022

ntrel commented Dec 10, 2022

ntrel Nov 17, 2022 •

edited

Loading

schveiguy Nov 19, 2022 •

edited

Loading

schveiguy Nov 19, 2022 •

edited

Loading

ntrel commented Dec 4, 2022 •

edited

Loading