[Rd] [External] use of the tcltk package crashes R 4.0.1 for Windows

[peter dalgaard]

Ah, I see it now: 

The remapping of free() to Rm_free() and calloc() to Rm_calloc() happens in memory.c, but not in tcltk.c; the macro Calloc in R_ext/RS.h maps to a call to R_chk_alloc which is defined in memory.h; RS.h is included in tcltk.c, so tcltk.c winds up calling Rm_calloc() via Calloc(), but then the NON-remapped free(), and the walls come tumbling down. 

If the  "#if defined(Win32)" block had been inside RS.h, the problem wouldn't arise.

-pd

> On 8 Jun 2020, at 00:03 , luke-tierney using uiowa.edu wrote:
> 
> 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 using gmail.com> wrote:
>>> 
>>> On Sun, Jun 7, 2020 at 5:53 PM <luke-tierney using 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 using uiowa.edu
> Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu

-- 
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 using cbs.dk  Priv: PDalgd using gmail.com