June 08, 2021

On Tuesday, 8 June 2021 at 08:11:38 UTC, Kagamin wrote:

>

On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:

>

an assert checking a magic

As I understand, this code unintentionally accesses thread local connection pool in multithreaded manner without synchronization?

https://github.com/vibe-d/vibe-http/blob/master/source/vibe/http/client.d#L216 and looks like on connection to 17th host the first pool is freed from the cache and later collected as garbage, so its lifetime is limited, but you touch it in destructor, which is UAF.

June 08, 2021
On 6/6/2021 4:54 AM, Mathias LANG wrote:
> https://github.com/dlang/druntime/pull/3476/files

Frankly, the whole jazz with assert() went way off the rails. Assert should go directly to Jail, it should not pass Go or collect $200. All these layers it goes through is just madness.

Allocating via the GC is in assert is just ridiculous, as the program is going to exit shortly anyway. There won't ever be a need to run a collection on it. Just malloc away and fuggeddabootet.

These sorts of things are why I added the checkaction switch to bypass it and just do the simple, direct stuff.

---

Throwing constructors is one of those things that makes a program very hard to reason about. I've debated with Andrei about requiring that constructors be nothrow. My advice is make them nothrow, i.e. design them so they cannot fail. There's not a single throwing constructor in DMD, and it's going to stay that way :-)

---

A destructor should never try to remove a GC allocated member. The GC may have already removed it! As with constructors, design destructors so they cannot fail. You'll have a lot less heartache that way.

---

This is not to argue that we don't need to fix the bugs. Thanks, Mathias, for the fixes you've created! It's appreciated.
June 09, 2021
On 6/6/21 7:54 AM, Mathias LANG wrote:
> Over the past few months, more than half of the critical bug we've encountered which were not due to our own code are related to destructors. Specifically, destructors being invoked by the GC.

Code in class destructors needs special typechecking and be restricted. IIRC, among other things it wasn't supposed to assume references to other class objects are valid. This has been discussed several times but never implemented.
June 09, 2021
On 6/8/21 10:37 PM, Walter Bright wrote:
> Throwing constructors is one of those things that makes a program very hard to reason about. I've debated with Andrei about requiring that constructors be nothrow. My advice is make them nothrow, i.e. design them so they cannot fail. There's not a single throwing constructor in DMD, and it's going to stay that way :-)

Except for FileMapping, and that's a Good Thing(tm):

https://github.com/dlang/dmd/pull/12560/files?file-filters%5B%5D=.c&file-filters%5B%5D=.d&file-filters%5B%5D=.md#diff-0be8539d9165e1607c7b47964ebf59a507c9ab436c6c4350df36d1c61d111a50R74

I'm half joking - that cosntructor may terminate the application.

Throwing constructors are an important part of achieving good encapsulation because they allow avoiding "invalid" states of objects altogether. In fairness, the fact that D has no user-definable default constructors weakens that argument (and is a weakness of the language itself that we'd do good to fix).
June 09, 2021
On Wednesday, 9 June 2021 at 02:37:34 UTC, Walter Bright wrote:
> On 6/6/2021 4:54 AM, Mathias LANG wrote:
> > https://github.com/dlang/druntime/pull/3476/files
>
> Frankly, the whole jazz with assert() went way off the rails. Assert should go directly to Jail, it should not pass Go or collect $200. All these layers it goes through is just madness.
>
> Allocating via the GC is in assert is just ridiculous, as the program is going to exit shortly anyway. There won't ever be a need to run a collection on it. Just malloc away and fuggeddabootet.
>
> These sorts of things are why I added the checkaction switch to bypass it and just do the simple, direct stuff.
>
> ---
>
> Throwing constructors is one of those things that makes a program very hard to reason about.

Placing (part of) constructor logic into a separate function that may throw, and pretending that a partially constructed state is a valid state - why do you think it is better?

> There's not a single throwing constructor in DMD, and it's going to stay that way :-)
>

Yeah, because they are not constructors. I bet you either construct lazily when a throwing public method is called, or require an explicit call to a throwing pseudo-constructor.
June 09, 2021
On 6/8/21 10:37 PM, Walter Bright wrote:
> On 6/6/2021 4:54 AM, Mathias LANG wrote:
>  > https://github.com/dlang/druntime/pull/3476/files
> 
> Frankly, the whole jazz with assert() went way off the rails. Assert should go directly to Jail, it should not pass Go or collect $200. All these layers it goes through is just madness.
> 
> Allocating via the GC is in assert is just ridiculous, as the program is going to exit shortly anyway. There won't ever be a need to run a collection on it. Just malloc away and fuggeddabootet.

Come to think of it, an InvalidMemoryOperationError should use malloc instead of GC, then maybe we can get stack traces?

Note that fixing asserts so they don't GC-allocate is just masking what the true problem here is -- any InvalidMemoryOperationError reports zero context for why it was thrown. In *this case* it happened to be an assert inside a destructor. But there are other times where an inadvertent destructor allocation causes an error, and it's impossible to find the spot where it was triggered without using a debugger.

-Steve
June 09, 2021
On 6/9/21 7:02 AM, Steven Schveighoffer wrote:
> Come to think of it, an InvalidMemoryOperationError should use malloc instead of GC, then maybe we can get stack traces?

I delved into this a bit, trying to see where the GC allocations are happening. There are some functions that are not marked nogc that can be.

I got hung up on core.demangle.demangle.

It's used here: https://github.com/dlang/druntime/blob/751f5665a31ea0f150d71b22a9852abacc2f61ec/src/core/runtime.d#L830-L865

Ironically, it demangles the string into a buffer, and then truncates that allocated buffer if it was allocated on the heap into the static buffer (and essentially throws away all the work if truncated).

We can possibly fix this by telling the demangler to use malloc (maybe in a nogc version), or tell the demangler to truncate for us if it runs out of room in the buffer (preferred).

The demangler is quite complex, I tried something cute by making a type that is a fake unlimited size array (but doesn't write to anything past the buffer), but it was more trouble than it's worth.

I'm 99% sure we could have a nogc stack trace for InvalidMemoryOperationError if we had a nogc demangler. Everything else seems already nogc, just not fully marked.

-Steve
June 09, 2021
On Wed, Jun 09, 2021 at 12:58:49AM -0400, Andrei Alexandrescu via Digitalmars-d wrote: [...]
> In fairness, the fact that D has no user-definable default constructors weakens that argument (and is a weakness of the language itself that we'd do good to fix).

This only applies to structs. Classes do have user-definable default ctors.

Nevertheless, the lack of user-definable default ctors in structs has been a frequent complaint.


T

-- 
Latin's a dead language, as dead as can be; it killed off all the Romans, and now it's killing me! -- Schoolboy
June 09, 2021
On 09.06.21 04:37, Walter Bright wrote:
> 
> Allocating via the GC is in assert is just ridiculous, as the program is going to exit shortly anyway. There won't ever be a need to run a collection on it. Just malloc away and fuggeddabootet.

AssertError is still *caught* /by the language/ on in contract inheritance.
June 09, 2021
On 6/8/2021 9:58 PM, Andrei Alexandrescu wrote:
> Throwing constructors are an important part of achieving good encapsulation because they allow avoiding "invalid" states of objects altogether. In fairness, the fact that D has no user-definable default constructors weakens that argument (and is a weakness of the language itself that we'd do good to fix).

Hence our disagreement :-)

BTW, over the years I've been evolving towards the notion that exception handling is a mistake.