Issue 14696 - destructor for temporary called before statement is complete with conditional operator
Summary: destructor for temporary called before statement is complete with conditional...
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P1 major
Assignee: No Owner
URL:
Keywords: pull, wrong-code
Depends on:
Blocks: 14979
  Show dependency treegraph
 
Reported: 2015-06-14 02:23 UTC by Steven Schveighoffer
Modified: 2019-11-29 00:33 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Steven Schveighoffer 2015-06-14 02:23:27 UTC
If a type's alias this is used as the parameter to a function, and that instance is a temporary, it should not be destroyed before the call occurs.

However, if that type is part of ternary operator, the dtor is called FIRST. Example:

import std.stdio;
struct S
{
    void *get() {return null;}
    ~this() { writeln("S.dtor");}
    alias get this;
}

S makes() { return S();}

void foo(void *x) { writeln("foo"); }

void main(string[] args)
{
    foo(args.length ? makes() : null);
}

The output:

S.dtor
foo

If, for example, S's dtor destroys the memory that is returned by get(), then this will result in dangling pointer to destroyed memory being passed to foo.

Real life example: https://github.com/D-Programming-Language/phobos/pull/3404#issuecomment-111704941

I'm uncertain what is required to make this happen. If I remove the ternary expression, then the destructor is properly called after foo. But I don't know if alias this is required.
Comment 1 Ketmar Dark 2015-06-14 04:40:46 UTC
the same bug on HEAD without an alias:

  //alias get this;
  ...
  foo(args.length ? makes().get : null);
Comment 2 Steven Schveighoffer 2015-06-15 12:40:09 UTC
(In reply to Ketmar Dark from comment #1)
> the same bug on HEAD without an alias:
> 
>   //alias get this;
>   ...
>   foo(args.length ? makes().get : null);

And the obvious smacks me in the face :) Thanks.
Comment 3 Walter Bright 2015-06-16 20:19:58 UTC
A simpler demonstration:

import core.stdc.stdio;

struct S {
    this(int i) {
        c = 's';
        p = &c;
    }

    ~this() {
        printf("S.dtor\n");
        c = 'd';
    }

    char *p;
    char c = 's';
}

int main() {
    char t = 't';
    char *q = &t;
    int x = 1;
    char *p = x ? S(1).p : q;
    printf("*p = %c\n", *p);
    assert(*p == 's');
    return 0;
}
Comment 4 Steven Schveighoffer 2015-06-16 21:08:15 UTC
(In reply to Walter Bright from comment #3)
> struct S {
>     this(int i) {
>         c = 's';
>         p = &c;

This is not allowed, and I'm not sure it's a valid test case. You could fix by doing this:

struct S
{
    this(ref char x){
        p = &x;
        *p = 's';
    }
    ~this() {
        if(p) *p = 'd';
        p = null;
    }
}

void main()
{
   char c = 'c';
   char o = 'o';
   int i = 1;
   writeln(*(i ? S(c).p : &o));
}

prints: d
Comment 5 Yuxuan Shui 2015-06-17 00:23:58 UTC
I think this one might be related: #14653
Comment 6 Yuxuan Shui 2015-06-17 00:25:08 UTC
(In reply to Yuxuan Shui from comment #5)
> I think this one might be related: #14653

https://issues.dlang.org/show_bug.cgi?id=14653
Comment 8 Ketmar Dark 2015-06-17 12:39:10 UTC
(In reply to Walter Bright from comment #3)
>     this(int i) {
>         c = 's';
>         p = &c;
>     }

compiler should play a crybaby here, as storing pointer to struct field is not valid for non-heap-allocated structs. considering that most structs are non-heap, i prefer to see at least a warning. which, for example, can be silenced with explicit cast: `p = cast(char*)&c;`. or even with some library function like `p = (&c).assumeCorrect;`
Comment 9 Ketmar Dark 2015-06-17 12:49:49 UTC
(In reply to Yuxuan Shui from comment #5)
> I think this one might be related: #14653
it is related in the sense that it doesn't do struct dtor in the right place, but the exact place where it is failed is different. ;-)
Comment 10 github-bugzilla 2015-09-02 08:56:37 UTC
Commits pushed to stable at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/3bbe72b5fdc278fe1293c20b39af4f94b10070bb
fix Issue 14696 - destructor for temporary called before statement is complete with conditional operator

https://github.com/D-Programming-Language/dmd/commit/38d31b0364e555ce42a8f47ca656e59b1cf90aad
Merge pull request #5003 from 9rnsr/fix14696_cdmd

Issue 14696, 14708, and [REG2.068] 14979 - Fix destructor issues for temporary in conditional operator
Comment 11 github-bugzilla 2015-09-07 13:37:01 UTC
Commits pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/3bbe72b5fdc278fe1293c20b39af4f94b10070bb
fix Issue 14696 - destructor for temporary called before statement is complete with conditional operator

https://github.com/D-Programming-Language/dmd/commit/38d31b0364e555ce42a8f47ca656e59b1cf90aad
Merge pull request #5003 from 9rnsr/fix14696_cdmd
Comment 12 Suleyman Sahmi (سليمان السهمي) 2019-11-28 14:15:45 UTC
The following case is still affected:

```
extern(C) void puts(const char*);

struct S
{
    int i;
    ~this() { puts("D"); }
}

S makeS(int i) { return S(i); }

void main()
{
    int var = true ? 1 : makeS(makeS(1).i - 1).i;
}
```

The program prints "D" at runtime which means the destructor is called when it shouldn't.
Comment 13 Dlang Bot 2019-11-28 15:11:59 UTC
@SSoulaimane created dlang/dmd pull request #10628 "Fix issue 14696 - Fix a remaining case where the destructor is still called on a cold branch" fixing this issue:

- Fix issue 14696 - Fix a remaining case where the destructor is still called on a cold branch
  
  Temporary variables should not be destroyed when the branch that initializes them is not taken.

https://github.com/dlang/dmd/pull/10628
Comment 14 Dlang Bot 2019-11-29 00:33:50 UTC
dlang/dmd pull request #10628 "Fix issue 14696 - Fix a remaining case where the destructor is still called on a cold branch" was merged into master:

- 2ec3f1090a11c5b6fe0a9507ba47f7081161ba7f by سليمان السهمي  (Suleyman Sahmi):
  Fix issue 14696 - Fix a remaining case where the destructor is still called on a cold branch
  
  Temporary variables should not be destroyed when the branch that initializes them is not taken.

https://github.com/dlang/dmd/pull/10628