R devel - Jun 2020 - [External] Re: use of the tcltk package crashes R 4.0.1 for Windows

On Sun, Jun 7, 2020 at 5:53 PM <luke-tierney at uiowa.edu>
wrote:
>
> On Sun, 7 Jun 2020, peter dalgaard wrote:
>
> > So this wasn't tested for a month?
> >
> > Anyways, Free() is just free() with a check that we're not freeing
a null pointer, followed by setting the pointer to NULL. At that point of
tcltk.c, we have
> >
> >   for (objc = i = 0; i < length(avec); i++){
> >        const char *s;
> >        char *tmp;
> >        if (!isNull(nm) && strlen(s =
translateChar(STRING_ELT(nm, i)))){
> >            //  tmp = calloc(strlen(s)+2, sizeof(char));
> >            tmp = Calloc(strlen(s)+2, char);
> >            *tmp = '-';
> >            strcpy(tmp+1, s);
> >            objv[objc++] = Tcl_NewStringObj(tmp, -1);
> >            free(tmp);
> >        }
> >        if (!isNull(t = VECTOR_ELT(avec, i)))
> >            objv[objc++] = (Tcl_Obj *) R_ExternalPtrAddr(t);
> >    }
> >
> > and I can't see how tmp can be NULL at the free(), nor can I see
it mattering if it is not set to NULL (notice that it goes out of scope with the
for loop).
>
> Right. And the calloc->Calloc change doesn't look like an issue
either
> -- just checking for a NULL.
>
> If the crash is happening in free() then that most likely means
> corrupted malloc data structures. Unfortunately that could be
> happening anywhere.
Writing R extensions, section 6.1.2 says: "Do not assume that memory
allocated by Calloc/Realloc comes from the same pool as used by
malloc: in particular do not use free or strdup with it."

I think the reason is that R uses dlmalloc for Calloc on Windows:
https://github.com/wch/r-source/blob/c634fec5214e73747b44d7c0e6f047fefe44667d/src/main/memory.c#L94-L103

> On 7 Jun 2020, at 18:59 , Jeroen Ooms <jeroenooms at gmail.com>
wrote:
> 
> On Sun, Jun 7, 2020 at 5:53 PM <luke-tierney at uiowa.edu> wrote:
>> 
>> On Sun, 7 Jun 2020, peter dalgaard wrote:
>> 
>>> So this wasn't tested for a month?
>>> 
>>> Anyways, Free() is just free() with a check that we're not
freeing a null pointer, followed by setting the pointer to NULL. At that point
of tcltk.c, we have
>>> 
>>>  for (objc = i = 0; i < length(avec); i++){
>>>       const char *s;
>>>       char *tmp;
>>>       if (!isNull(nm) && strlen(s =
translateChar(STRING_ELT(nm, i)))){
>>>           //  tmp = calloc(strlen(s)+2, sizeof(char));
>>>           tmp = Calloc(strlen(s)+2, char);
>>>           *tmp = '-';
>>>           strcpy(tmp+1, s);
>>>           objv[objc++] = Tcl_NewStringObj(tmp, -1);
>>>           free(tmp);
>>>       }
>>>       if (!isNull(t = VECTOR_ELT(avec, i)))
>>>           objv[objc++] = (Tcl_Obj *) R_ExternalPtrAddr(t);
>>>   }
>>> 
>>> and I can't see how tmp can be NULL at the free(), nor can I
see it mattering if it is not set to NULL (notice that it goes out of scope with
the for loop).
>> 
>> Right. And the calloc->Calloc change doesn't look like an issue
either
>> -- just checking for a NULL.
>> 
>> If the crash is happening in free() then that most likely means
>> corrupted malloc data structures. Unfortunately that could be
>> happening anywhere.
> 
> Writing R extensions, section 6.1.2 says: "Do not assume that memory
> allocated by Calloc/Realloc comes from the same pool as used by
> malloc: in particular do not use free or strdup with it."
> 
> I think the reason is that R uses dlmalloc for Calloc on Windows:
>
https://github.com/wch/r-source/blob/c634fec5214e73747b44d7c0e6f047fefe44667d/src/main/memory.c#L94-L103
But that section #defines calloc and free to Rm_... counterparts in lockstep? (I
assume that is where dlmalloc comes in?)

Anyways, does it actually work to change free() to Free()? If so, then all this
post mortem analysis is rather a moot point.

-pd

-- 
Peter Dalgaard, Professor,
Center for Statistics, Copenhagen Business School
Solbjerg Plads 3, 2000 Frederiksberg, Denmark
Phone: (+45)38153501
Office: A 4.23
Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com

FWIW, you can "test" this by using the daily R builds. They are here:
https://ci.appveyor.com/project/jeroen/base/history

This is the build before the change, it does not crash:
https://ci.appveyor.com/project/jeroen/base/builds/32781690

This is the build right after the change, it does crash:
https://ci.appveyor.com/project/jeroen/base/builds/32808414

(To get the installers, click on "Artifacts" in the top right corner,
download the zip file, it has an exe installer.)

There are a handful of commits between the two installers, but only
this change seems to be related to the crash:
https://github.com/wch/r-source/commit/ba76508c8041335c1896df491ac227fdde0cfb0d
https://github.com/wch/r-source/commit/2c047b94bfe0996419f8871ed6b62b1e7d5ec7bd
https://github.com/wch/r-source/commit/59840c37eb20e05241fb9e85228331fa31db9a2b
https://github.com/wch/r-source/commit/161fc3f703e46201299e87bf7717a93d13c23970
https://github.com/wch/r-source/commit/4c267df39d776fa10c4b2d6b3872dbb85b073681
https://github.com/wch/r-source/commit/f3de035e12a8c8920772297405ed25ee6b3ec4a6

Gabor

On Sun, Jun 7, 2020 at 6:40 PM peter dalgaard <pdalgd at gmail.com>
wrote:
>
>
>
> > On 7 Jun 2020, at 18:59 , Jeroen Ooms <jeroenooms at gmail.com>
wrote:
> >
> > On Sun, Jun 7, 2020 at 5:53 PM <luke-tierney at uiowa.edu>
wrote:
> >>
> >> On Sun, 7 Jun 2020, peter dalgaard wrote:
> >>
> >>> So this wasn't tested for a month?
> >>>
> >>> Anyways, Free() is just free() with a check that we're not
freeing a null pointer, followed by setting the pointer to NULL. At that point
of tcltk.c, we have
> >>>
> >>>  for (objc = i = 0; i < length(avec); i++){
> >>>       const char *s;
> >>>       char *tmp;
> >>>       if (!isNull(nm) && strlen(s =
translateChar(STRING_ELT(nm, i)))){
> >>>           //  tmp = calloc(strlen(s)+2, sizeof(char));
> >>>           tmp = Calloc(strlen(s)+2, char);
> >>>           *tmp = '-';
> >>>           strcpy(tmp+1, s);
> >>>           objv[objc++] = Tcl_NewStringObj(tmp, -1);
> >>>           free(tmp);
> >>>       }
> >>>       if (!isNull(t = VECTOR_ELT(avec, i)))
> >>>           objv[objc++] = (Tcl_Obj *) R_ExternalPtrAddr(t);
> >>>   }
> >>>
> >>> and I can't see how tmp can be NULL at the free(), nor can
I see it mattering if it is not set to NULL (notice that it goes out of scope
with the for loop).
> >>
> >> Right. And the calloc->Calloc change doesn't look like an
issue either
> >> -- just checking for a NULL.
> >>
> >> If the crash is happening in free() then that most likely means
> >> corrupted malloc data structures. Unfortunately that could be
> >> happening anywhere.
> >
> > Writing R extensions, section 6.1.2 says: "Do not assume that
memory
> > allocated by Calloc/Realloc comes from the same pool as used by
> > malloc: in particular do not use free or strdup with it."
> >
> > I think the reason is that R uses dlmalloc for Calloc on Windows:
> >
https://github.com/wch/r-source/blob/c634fec5214e73747b44d7c0e6f047fefe44667d/src/main/memory.c#L94-L103
>
> But that section #defines calloc and free to Rm_... counterparts in
lockstep? (I assume that is where dlmalloc comes in?)
>
> Anyways, does it actually work to change free() to Free()? If so, then all
this post mortem analysis is rather a moot point.
>
> -pd
>
> --
> Peter Dalgaard, Professor,
> Center for Statistics, Copenhagen Business School
> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
> Phone: (+45)38153501
> Office: A 4.23
> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

I've committed the change to use Free instead of free in tcltk.c and
sys-std.c (r78652 for R-devel, r78653 for R-patched).

We might consider either moving Calloc/Free out of the Windows
remapping or moving the remapping into header files so everything
seeing our header files uses our calloc/free. Either would be less
brittle that the current status.

Best,

luke

On Sun, 7 Jun 2020, peter dalgaard wrote:
>
>
>> On 7 Jun 2020, at 18:59 , Jeroen Ooms <jeroenooms at gmail.com>
wrote:
>>
>> On Sun, Jun 7, 2020 at 5:53 PM <luke-tierney at uiowa.edu> wrote:
>>>
>>> On Sun, 7 Jun 2020, peter dalgaard wrote:
>>>
>>>> So this wasn't tested for a month?
>>>>
>>>> Anyways, Free() is just free() with a check that we're not
freeing a null pointer, followed by setting the pointer to NULL. At that point
of tcltk.c, we have
>>>>
>>>>  for (objc = i = 0; i < length(avec); i++){
>>>>       const char *s;
>>>>       char *tmp;
>>>>       if (!isNull(nm) && strlen(s =
translateChar(STRING_ELT(nm, i)))){
>>>>           //  tmp = calloc(strlen(s)+2, sizeof(char));
>>>>           tmp = Calloc(strlen(s)+2, char);
>>>>           *tmp = '-';
>>>>           strcpy(tmp+1, s);
>>>>           objv[objc++] = Tcl_NewStringObj(tmp, -1);
>>>>           free(tmp);
>>>>       }
>>>>       if (!isNull(t = VECTOR_ELT(avec, i)))
>>>>           objv[objc++] = (Tcl_Obj *) R_ExternalPtrAddr(t);
>>>>   }
>>>>
>>>> and I can't see how tmp can be NULL at the free(), nor can
I see it mattering if it is not set to NULL (notice that it goes out of scope
with the for loop).
>>>
>>> Right. And the calloc->Calloc change doesn't look like an
issue either
>>> -- just checking for a NULL.
>>>
>>> If the crash is happening in free() then that most likely means
>>> corrupted malloc data structures. Unfortunately that could be
>>> happening anywhere.
>>
>> Writing R extensions, section 6.1.2 says: "Do not assume that
memory
>> allocated by Calloc/Realloc comes from the same pool as used by
>> malloc: in particular do not use free or strdup with it."
>>
>> I think the reason is that R uses dlmalloc for Calloc on Windows:
>>
https://github.com/wch/r-source/blob/c634fec5214e73747b44d7c0e6f047fefe44667d/src/main/memory.c#L94-L103
>
> But that section #defines calloc and free to Rm_... counterparts in
lockstep? (I assume that is where dlmalloc comes in?)
>
> Anyways, does it actually work to change free() to Free()? If so, then all
this post mortem analysis is rather a moot point.
>
> -pd
>
>
-- 
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa                  Phone:             319-335-3386
Department of Statistics and        Fax:               319-335-3017
    Actuarial Science
241 Schaeffer Hall                  email:   luke-tierney at uiowa.edu
Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu