Fix issue 19797 - Ungraceful termination in File.seek() on Windows

[veelo]

Context: https://forum.dlang.org/post/mqzblhbrweydjlcjdppj@forum.dlang.org

1. Extra parameter validation when compiling for CRuntime_Microsoft to prevent ungraceful process termination. Fixes issue 19797. This is a workaround for what looks like a bug in Microsoft's _fseeki64.
2. Improved documentation.

[dlang-bot]

Thanks for your pull request and interest in making D better, @veelo! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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

Auto-close	Bugzilla	Severity	Description
✗	19797	critical	File.seek() terminates ungracefully on incorrect origin for -m32mscoff and -m64

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#6948"

[CyberShadow]

I think this is a sensible approach to the problem, but I don't know how it stacks up to the cost of the added code, considering that std.stdio is unashamed about being a D-ish wrapper around stdio.h. For instance, we haven't replaced the fopen mode string with an enum, but left it as a string (though a D string instead of a char*). We used to have std.stream, which was a more abstract I/O interface, but it was unused (and had questionable design choices), so it was dropped to UndeaD.

I think the overload in support of legacy code could be removed if the SeekFrom enum were moved to core.stdc.stdio with aliases to it for SEEK_SET, SEEK_CUR and SEEK_END.

I understand core.stdc is meant to be a 1:1 translation of C header files, so that wouldn't be appropriate.

[veelo]

Thanks for chiming in.

I think this is a sensible approach to the problem, but I don't know how it stacks up to the cost of the added code, considering that std.stdio is unashamed about being a D-ish wrapper around stdio.h.

I don't really like it either. An alternative is to basically keep the code as is, but add assert(origin >= SEEK_STEP && origin <= SEEK_END). Maybe behind version(Windows) as at least run.dlang.io throws an ErrnoException for invalid origins. And add proper documentation.

I've tried to do the check at CT, since they are CT constants, but I couldn't make that work.

[CyberShadow]

And add proper documentation.

We can always do that.

An alternative is to basically keep the code as is, but add assert(origin >= SEEK_STEP && origin <= SEEK_END).

Not sure what this would achieve, does the C runtime you used not return an error if a bad value was specified?

If std.stdio does not check the return value of some C function, then that is an obvious bug fixing which would solve this.

[veelo]

An alternative is to basically keep the code as is, but add assert(origin >= SEEK_STEP && origin <= SEEK_END).

Not sure what this would achieve, does the C runtime you used not return an error if a bad value was specified?

The process just terminated without any message whatsoever.

If std.stdio does not check the return value of some C function, then that is an obvious bug fixing which would solve this.

I'll take a closer look, thanks.

[CyberShadow]

The process just terminated without any message whatsoever.

That should never happen. Something must have went terribly wrong.

If you don't want to dig into that but can create a reproducible example, please post it on Bugzilla.

[veelo]

Could there be a difference in behaviour between fseek and _fseeki64? The problem does not occur for -m32, only with -m32mscoff and -m64 (on Windows).

https://issues.dlang.org/show_bug.cgi?id=19797

import std.experimental.all;

void main()
{       
        auto deleteme = "deleteme";
        auto f = File(deleteme, "wb+");
        scope(exit) { f.close(); std.file.remove(deleteme); }
        f.seek(0, 3);   // Should throw.

        writeln("Ending gracefully."); // Never seen.
}

I'll have to let this rest for a moment.

[veelo]

I'm done. The one failure seems to be a random hickup?

timelimit: sending warning signal 15
make[1]: *** [unittest/std/datetime/systime.run] Error 143
make[1]: Leaving directory `/home/ec2-user/sandbox/at-client/pull-3594245-Linux_32_64/phobos'
make: *** [unittest-debug] Error 2

The previous build went through, and this last one is just a documentation fix. Can someone just restart the auto-tester or something?

Thanks.

[wilzbach]

Marked the build as deprecated. It will take a bit for it to rerun, but once it has the auto-merge tag the Auto-Tester will actually constantly rerun its tests until it can merge.

[thewilsonator]

@veelo could you please clean up the commit history, as soon as you do this can be merged.

[veelo]

@veelo could you please clean up the commit history, as soon as you do this can be merged.

@thewilsonator I had to learn how to do that first, I think I've got it. :-)

[Geod24]

This caused some problems for porting to Musl. Value 3 is actually SEEK_DATA on Linux systems (http://man7.org/linux/man-pages/man2/lseek.2.html), which is a valid value.
Using a definitely invalid value (like int.max or int.max / 2) would have been better.