Bug 17102 – std.write.file generates a segmentation fault when the file name is a string with a default value
Status
RESOLVED
Resolution
FIXED
Severity
normal
Priority
P1
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2017-01-17T03:07:00Z
Last change time
2017-03-22T12:21:50Z
Assigned to
nobody
Creator
donte5379
Comments
Comment #0 by donte5379 — 2017-01-17T03:07:57Z
The following program generates a segmentation fault with rdmd or compiling with dmd and running the resulting executable. The data is a valid string and the file name is a string that is left at the default value.
#!/usr/local/bin/rdmd
import std.stdio;
import std.file;
void main (string[] args) {
string the_text = "File with bad name"; // some data to write
string file_name; // set to default string value
std.file.write(file_name, the_text);
}
% dmd --version
DMD64 D Compiler v2.072.2
Copyright (c) 1999-2016 by Digital Mars written by Walter Bright
%
OSX 10.11.5 (15F34)
Comment #1 by dfj1esp02 — 2017-01-20T12:20:22Z
It depends on what you want. If we treat the default string value as null, it gets passed as null to C API, and the crash is expected.
Comment #2 by donte5379 — 2017-01-20T16:57:06Z
I think that std.file.write should throw an exception before the null gets passed to the C API. Even if I add a try-catch block all I see is a message "Segmentation fault" on the terminal.
std.file.write(file_name, the_text);
} catch (Exception e){
writeln(e);
}
As an added frustration the redirection is corrupted in bash (and tcsh) and so it is difficult to run dustmite. I ended up reducing the failing case by hand.
Comment #3 by dlang-bugzilla — 2017-01-20T17:24:44Z
(In reply to donte5379 from comment #2)
> I think that std.file.write should throw an exception before the null gets
> passed to the C API.
I don't think it's the job of the standard library to validate that the input parameter is valid before passing it to a C API.
> Even if I add a try-catch block all I see is a message
> "Segmentation fault" on the terminal.
>
> std.file.write(file_name, the_text);
> } catch (Exception e){
> writeln(e);
> }
Writing to a file whose file name is unset is a logic error in the user's code, so if such a check would be present in std.file.write, its correct behavior would be to throw an Error, which would also bypass your try/catch block (as you only catch an Exception), so the program would behave similarly.
The main difference would be that on POSIX, by default the D runtime doesn't attempt to handle and print a stack trace on unhandled signals (which would otherwise terminate the program). On Linux, there exists etc.linux.memoryerror, which translates segmentation faults to D exceptions. On Windows, access violations are already translated to D exceptions through D's support for Windows structured exception handling.
Either way, finding the cause of the segmentation fault would have been as simple as running your program under a debugger.
> As an added frustration the redirection is corrupted in bash (and tcsh) and
> so it is difficult to run dustmite.
The DustMite documentation contains examples on correctly reducing segmentation faults. I've also created a page to explain the basics: https://github.com/CyberShadow/DustMite/wiki/Reducing-a-segmentation-fault
Comment #4 by jack — 2017-01-20T20:41:02Z
(In reply to Vladimir Panteleev from comment #3)
> I don't think it's the job of the standard library to validate that the
> input parameter is valid before passing it to a C API.
We can at least add an assert with an error message.
> Either way, finding the cause of the segmentation fault would have been as
> simple as running your program under a debugger.
Impossible with macOS.
Comment #5 by dlang-bugzilla — 2017-01-20T20:42:51Z
(In reply to Jack Stouffer from comment #4)
> Impossible with macOS.
How so?
Comment #7 by dlang-bugzilla — 2017-01-20T21:27:30Z
(In reply to Jack Stouffer from comment #6)
> (In reply to Vladimir Panteleev from comment #5)
> > (In reply to Jack Stouffer from comment #4)
> > > Impossible with macOS.
> >
> > How so?
>
> https://issues.dlang.org/show_bug.cgi?id=14927 and
> https://issues.dlang.org/show_bug.cgi?id=8172
Those are implementation issues orthogonal to this one.
(In reply to Jack Stouffer from comment #4)
> We can at least add an assert with an error message.
- Adding an assert won't work, because the Phobos static library is compiled with -release.
- Adding an explicit if (...) throw new Error(...) will not throw an exception that would have been caught in #2.
- I still think it's not Phobos' job to validate parameters passed from the user to the C runtime. It is illogical. Consider:
- Explicit checks for parameters that will get dereferenced anyway will be redundant and will add overhead in all cases except when the program is buggy.
- D functions in the runtime and standard library which accept reference types do not perform explicit checks whether a parameter that should never be null is non-null - dereferencing implicitly does that.
- I don't see a meaningful distinction in whether null checks should be present depending on whether the dereference occurs in D or C code.