Allocating with new but freeing the memory with free

[Paul]

The code below is from Hackerrank. It looks suspicious to me because
the memory is allocated with new but freed with free.

Is it ok?

Thanks,

Paul


using namespace std;

class SinglyLinkedListNode {
public:
int data;
SinglyLinkedListNode *next;

SinglyLinkedListNode(int node_data) {
this->data = node_data;
this->next = nullptr;
}
};

class SinglyLinkedList {
public:
SinglyLinkedListNode *head;
SinglyLinkedListNode *tail;

SinglyLinkedList() {
this->head = nullptr;
this->tail = nullptr;
}

void insert_node(int node_data) {
SinglyLinkedListNode* node = new SinglyLinkedListNode(node_data);

if (!this->head) {
this->head = node;
} else {
this->tail->next = node;
}

this->tail = node;
}
};


void free_singly_linked_list(SinglyLinkedListNode* node) {
while (node) {
SinglyLinkedListNode* temp = node;
node = node->next;

free(temp);
}
}

[Öö Tiib]

On Friday, 12 October 2018 22:09:22 UTC+3, Paul wrote:
> The code below is from Hackerrank. It looks suspicious to me because
> the memory is allocated with new but freed with free.
>
> Is it ok?

Calling free on pointers that aren't null and weren't returned from
aligned_alloc, calloc, malloc or realloc is undefined behavior AFAIK.
Also free may not call delete (like those others may not call new).
However users can guarantee that new returns the pointers from one of
those functions by globally overloading new.

[Ian Collins]

On 13/10/18 08:09, Paul wrote:
> The code below is from Hackerrank. It looks suspicious to me because
> the memory is allocated with new but freed with free.
>
> Is it ok?

No!

The pointer returned by new may not have been allocated with malloc, so
passing it to free is undefined behaviour. Also calling free will not
invoke an object's destructor.

--
Ian.

[Alf P. Steinbach]

On 12.10.2018 21:09, Paul wrote:
> The code below is from Hackerrank. It looks suspicious to me because
> the memory is allocated with new but freed with free.
>
> Is it ok?

For portable code it's just Undefined Behavior.

However, what's formal UB can be useful defined behavior with a given
implementation. It's often exploited by the (compiler-specific)
implementation of the standard library, e.g. in the `offsetof` code. But
the ordinary programmer has to consider several levels of defined-ness:

* Defined by the C++ standard.
* Defined by a large number of compilers (e.g. `#pragma once`, or
`__COUNTER__`), a /de facto/ standard. However don't use that term over
on Stack Overflow. They're too lazy, or maybe arrogant, to look up
things they don't know, and I note that the care that one must exercise
there has been adopted over on cppreference, which doesn't talk about de
facto standards.
* Defined by the compiler, and documented.
* Defined by the compiler, but not documented (e.g., that you've found
code from the compiler vendor that relies on some specific behavior).
* No source other than your own testing indicates that this thing works.

Usually, in discussions about C++ portable code is assumed, and so only
code that passes via the first and second points above is considered OK.

But one's portability goals can be much more limited, and for example,
but even though I recall one such situation this must be very very rare,
one may have to pass a `new`-allocated thing to a C routine that will
`free` whatever it gets, and for efficiency avoid an extra `malloc` +
copying. Then one may decide to go with one of the two last points. But
I think that should be documented: "NOTE: NOT USING STRDUP BECAUSE
MEASUREMENTS SHOWED THAT WAS TOO SLOW, AND BORKA C++ NEW /IS/ MALLOC".


Cheers & hth.,

- Alf

[Öö Tiib]

On Saturday, 13 October 2018 05:24:29 UTC+3, Alf P. Steinbach wrote:
> On 12.10.2018 21:09, Paul wrote:
> > The code below is from Hackerrank. It looks suspicious to me because
> > the memory is allocated with new but freed with free.
> >
> > Is it ok?
>
> For portable code it's just Undefined Behavior.
>
> However, what's formal UB can be useful defined behavior with a given
> implementation. It's often exploited by the (compiler-specific)
> implementation of the standard library, e.g. in the `offsetof` code.

Yes, but that standard library may change with next patch. Are there
any ways to detect in code that it did?

> But
> the ordinary programmer has to consider several levels of defined-ness:
>
> * Defined by the C++ standard.
> * Defined by a large number of compilers (e.g. `#pragma once`, or
> `__COUNTER__`), a /de facto/ standard. However don't use that term over
> on Stack Overflow. They're too lazy, or maybe arrogant, to look up
> things they don't know, and I note that the care that one must exercise
> there has been adopted over on cppreference, which doesn't talk about de
> facto standards.
> * Defined by the compiler, and documented.
> * Defined by the compiler, but not documented (e.g., that you've found
> code from the compiler vendor that relies on some specific behavior).
> * No source other than your own testing indicates that this thing works.

Major issue there is lack of standardized way to detect compile time
the implementation-defined, some-other-standard-defined, compiler-defined
or compiler-option-defined features. So portable (in practice) code is
often full of preprocessor slices and obscurely unreadable static_assert
conditions.

> Usually, in discussions about C++ portable code is assumed, and so only
> code that passes via the first and second points above is considered OK.
>
> But one's portability goals can be much more limited, and for example,
> but even though I recall one such situation this must be very very rare,
> one may have to pass a `new`-allocated thing to a C routine that will
> `free` whatever it gets, and for efficiency avoid an extra `malloc` +
> copying.

Hmm ... but anything that C library can use and free must be dynamically
allocated POD and so there had to be possibility to malloc or calloc
storage for it instead?

> Then one may decide to go with one of the two last points. But
> I think that should be documented: "NOTE: NOT USING STRDUP BECAUSE
> MEASUREMENTS SHOWED THAT WAS TOO SLOW, AND BORKA C++ NEW /IS/ MALLOC".

Willfully going with one of the last two when some other bullet
explicitly documented it as "undefined behavior" (as on current case)
is considered "black magic". It will be nuisance later when it leads
into one of several inconveniences and difficulties. Most typical
difficulties that manifest are:

* choice of other useful tools (and platforms for running those) is
limited
* portability to newer versions of compiler is harder
* certain very desirable compiler flags (like optimizations) break it
* further maintainability of module is harder

When such difficulties manifest then with 100% our own code it "just
sucks". With others we are guilty. Admit personal fallibility, agree
that it was programming defect and help to remove the difficulties.
Any argument can bring lost trust, reputation, contracts and fame as
the evil author of "cursed legacy". What must be the alleged benefits
that make it worth that? No one did monitor if the end users were
happy about it or not. ;)

[Bo Persson]

On 2018-10-13 12:37, Öö Tiib wrote:
> On Saturday, 13 October 2018 05:24:29 UTC+3, Alf P. Steinbach wrote:
>> On 12.10.2018 21:09, Paul wrote:
>>> The code below is from Hackerrank. It looks suspicious to me because
>>> the memory is allocated with new but freed with free.
>>>
>>> Is it ok?
>>
>> For portable code it's just Undefined Behavior.
>>
>> However, what's formal UB can be useful defined behavior with a given
>> implementation. It's often exploited by the (compiler-specific)
>> implementation of the standard library, e.g. in the `offsetof` code.
>
> Yes, but that standard library may change with next patch. Are there
> any ways to detect in code that it did?
>

In this particular case, all the major compilers now have a
__bultin_offsetof "magic" function.

So the old null-pointer hack might no longer be supported. Who knows?


Bo Persson

[Alf P. Steinbach]

Well, that brings up an interesting case, and an issue re definedness
that I forgot to address. I forget too much these days, sorry. Namely,
the case of

* defined by the standard, but not implemented or incorrectly
implemented by at least one major compiler.

When Petru Marginean and co-author Andrei Alexandrescu published the now
classic ¹Scope Guard article in Dr Dobbs Journal, they chose to use the
standard `__LINE__` macro to generate unique names for the scope guard
variables. They could have used de facto standard `__COUNTER__`, which I
think even at that time was supported by all the major compilers. The
choice of `__LINE__` meant that their code just didn't work with the
default settings of a Visual Studio project, because with the Visual C++
option for "Program Database for Edit and Continue", `/ZI`, Visual C++
expanded `__LINE__` to some gobbledygook, not valid in an identifier.

They never agreed that it was a programming defect, never fixed it. For
that matter, Andrei never agreed that it was a programming defect to
just silently swallow exceptions in the Scope Guard cleanup code. And
nobody now (well except me) remembers Petru Marginean... :)


Cheers!,

- Alf

Notes:
¹ <url:
http://www.drdobbs.com/cpp/generic-change-the-way-you-write-excepti/184403758>

[Öö Tiib]

That does also happen and sometimes such defects won't be fixed for
decade but usually it helps when to wait for about two years before
using some new feature in production code. Otherwise the difficulties
will be similar but accusations will be reverted: "Who cares what
standards say? We need to deliver working programs!"

>
> When Petru Marginean and co-author Andrei Alexandrescu published the now
> classic ¹Scope Guard article in Dr Dobbs Journal, they chose to use the
> standard `__LINE__` macro to generate unique names for the scope guard
> variables. They could have used de facto standard `__COUNTER__`, which I
> think even at that time was supported by all the major compilers. The
> choice of `__LINE__` meant that their code just didn't work with the
> default settings of a Visual Studio project, because with the Visual C++
> option for "Program Database for Edit and Continue", `/ZI`, Visual C++
> expanded `__LINE__` to some gobbledygook, not valid in an identifier.

Somehow it seems changed, or I can't find __LINE__ in it.

> They never agreed that it was a programming defect, never fixed it. For
> that matter, Andrei never agreed that it was a programming defect to
> just silently swallow exceptions in the Scope Guard cleanup code. And
> nobody now (well except me) remembers Petru Marginean... :)

Oh, he likely got tired of being public guru and works on some sort
of investment banking or cryptocurrency project.

> Cheers!,
>
> - Alf
>
> Notes:
> ¹ <url:
> http://www.drdobbs.com/cpp/generic-change-the-way-you-write-excepti/184403758>

Yes that one ... where they use __LINE__ there?

[Alf P. Steinbach]

On 13.10.2018 16:13, Öö Tiib wrote:
> On Saturday, 13 October 2018 16:03:58 UTC+3, Alf P. Steinbach wrote:

>> [snip]

>> When Petru Marginean and co-author Andrei Alexandrescu published the now
>> classic ¹Scope Guard article in Dr Dobbs Journal, they chose to use the
>> standard `__LINE__` macro to generate unique names for the scope guard
>> variables. They could have used de facto standard `__COUNTER__`, which I
>> think even at that time was supported by all the major compilers. The
>> choice of `__LINE__` meant that their code just didn't work with the
>> default settings of a Visual Studio project, because with the Visual C++
>> option for "Program Database for Edit and Continue", `/ZI`, Visual C++
>> expanded `__LINE__` to some gobbledygook, not valid in an identifier.
>
> Somehow it seems changed, or I can't find __LINE__ in it.
>
>> They never agreed that it was a programming defect, never fixed it. For
>> that matter, Andrei never agreed that it was a programming defect to
>> just silently swallow exceptions in the Scope Guard cleanup code. And
>> nobody now (well except me) remembers Petru Marginean... :)
>
> Oh, he likely got tired of being public guru and works on some sort
> of investment banking or cryptocurrency project.
>
>
>> Cheers!,
>>
>> - Alf
>>
>> Notes:
>> ¹ <url:
>> http://www.drdobbs.com/cpp/generic-change-the-way-you-write-excepti/184403758>
>
> Yes that one ... where they use __LINE__ there?

The source code was available as a .zip file. The article was not a
description of a technique. It was a description of a ready-to-use
mini-library, that later was incorporated into the Loki library.

The code was in two parts: Petru's original Scope Guard implementation,
which Andrei had helped improve, and some higher level wrapper stuff by
Andrei only, as I understood it.

Alas, it seems that the source code has been removed. At least it's not
in this list: <url: http://www.drdobbs.com/archives/sourcecode/2000>.


Cheers!,

- Alf

[Manfred]

I am with Öö here, I have never gone this way in production code, and I
am confident I never will - it is just waiting for trouble to happen.
By the way, there are not many C routines that do `free` on what they
get, and typically they are meant to be paired with other routines that
do `malloc` the appropriate object (an example is getaddrinfo/freeaddrinfo).
It is much too better to properly adjust the code, even if this means to
not call the "C routine that will `free` whatever it gets" and
reimplement it myself.
In my experience I even happened to encounter trouble with code where
some object was allocated with `new` in application code and freed with
`delete` by a library, so forcing `new` and `free` to pair would be a
no-go for me.

But
> I think that should be documented: "NOTE: NOT USING STRDUP BECAUSE
> MEASUREMENTS SHOWED THAT WAS TOO SLOW, AND BORKA C++ NEW /IS/ MALLOC".

This wouldn't be a 'should', it'd be a *must* instead, possibly
integrated with apologies in advance for the future maintainers...

[Richard]

[Please do not mail me a copy of your followup]

Paul <peps...@gmail.com> spake the secret code
<12c5b374-4edf-4d1b...@googlegroups.com> thusly:

>The code below is from Hackerrank. It looks suspicious to me because
>the memory is allocated with new but freed with free.
>
>Is it ok?

No. It is an error to mismatch allocation mechanisms. Commonly
mismatched pairs are:

- array new, scalar delete
- scalar new, array delete
- new, free
- malloc, delete
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
The Terminals Wiki <http://terminals-wiki.org>
The Computer Graphics Museum <http://computergraphicsmuseum.org>
Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>