Range-ify std.internal.cstring 2nd try #3415
Conversation
| else | ||
| { | ||
| To[256 / To.sizeof] _buff; // the 'small string optimization' | ||
| } |
There was a problem hiding this comment.
As I said in the original pull. This cannot be allowed. It changes the layout for importing modules based on whether -unittest is defined, but will link against the non-unittest version if it's using standard phobos library. This WILL cause buffer overflows, as the caller will reserve only 16 bytes for the stack space, but the function will run thinking it has 256 bytes.
I ran a simple test:
mod1.d:
import mod2;
void main()
{
auto x = foo();
}mod2.d:
module mod2;
auto foo() {
static struct S {
version(unittest)
char[16] x;
else
char[256] x;
}
return S();
}results:
$ dmd mod1.d mod2.d
$ ./mod1
$ dmd -c mod2.d
$ dmd -unittest mod1.d mod2.o
$ ./mod1
Segmentation fault: 11
There was a problem hiding this comment.
ok, I see what you mean. Will fix.
WalterBright
force-pushed
the
rangecstring2
branch
from
e931d22
to
48766b2
Compare
| // not confuse unprecise GC. | ||
| version (unittest) | ||
| { | ||
| enum buffLength = 16 / To.sizeof; // smaller size to trigger reallocations |
There was a problem hiding this comment.
the comment applies
There was a problem hiding this comment.
Oh, sorry, I missed that this is not actually used to set the size of the array, and thought it would still experience the issue Steven pointed out.
There was a problem hiding this comment.
Yeah, the private enum could be inside the function instead of the type, but I think there's no risk of this leaking outside the file somehow. So it works, thanks.
WalterBright
force-pushed
the
rangecstring2
branch
from
48766b2
to
ed1a063
Compare
|
One other thing: Do we want to have a char element type template constraint on the Range type? I would merge the pull request regardless because it is just an internal helper function at this point anyway, but the auto tester seems to be a bit slow on the uptake right now. |
| size_t i; | ||
|
|
||
| static if (isSomeString!From) | ||
| auto r = cast(const(CF)[])str; |
There was a problem hiding this comment.
I was curious about this on the last pull, but I didn't look at it until after it was merged so I didn't bother. But what is this part for?
There was a problem hiding this comment.
makes it happy when str is an inout.
There was a problem hiding this comment.
Ah, right. inout has many issues with generic code, we need to work on a solution for that.
|
Auto-merge toggled on |
Range-ify std.internal.cstring 2nd try
|
This pull request introduced a regression: |
The same code as for #3404. The https://issues.dlang.org/show_bug.cgi?id=14696 issue is not caused by this PR, and the existing cstring.d it replaces has the same latent issue. Also, 14696 should not hold up this PR, and the Phobos problem has been worked around with #3413