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

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

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 at 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 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
-- 
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

Jeroen, how "reactive" are the rtools40 scripts. Will they pull the
latest
version committed by Dr. Tierney or is there something which must be done
manually prior to we end-users rebuilding from source?

Thank you,

Avi

On Sun, Jun 7, 2020 at 11:01 PM peter dalgaard <pdalgd at gmail.com>
wrote:
> 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 at 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 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
>
> --
> 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
>
	[[alternative HTML version deleted]]

On Mon, Jun 8, 2020 at 12:03 AM <luke-tierney at 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).
Thank you! I can confirm that the example from above no longer crashes
in R--patched.

John, can you confirm that everything seems to work now in Rcmd with
today's R-patched build from CRAN?
https://cran.r-project.org/bin/windows/base/rpatched.html

Hopefully Peter will be able to tag a 4.0.2 hotfix release based on
4.0.1 + above patch, without going through the full release
procedure...

Dear Jeroen,

With the caveat that I've tested only a few of the Rcmdr dialogs (a full
test takes hours and must be done manually), everything seems to be working fine
again.

Thank you for addressing this problem so quickly.

John

  -----------------------------
  John Fox, Professor Emeritus
  McMaster University
  Hamilton, Ontario, Canada
  Web: http::/socserv.mcmaster.ca/jfox
> On Jun 8, 2020, at 3:36 AM, Jeroen Ooms <jeroenooms at gmail.com>
wrote:
> 
> On Mon, Jun 8, 2020 at 12:03 AM <luke-tierney at 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).
> 
> Thank you! I can confirm that the example from above no longer crashes
> in R--patched.
> 
> John, can you confirm that everything seems to work now in Rcmd with
> today's R-patched build from CRAN?
> https://cran.r-project.org/bin/windows/base/rpatched.html
> 
> Hopefully Peter will be able to tag a 4.0.2 hotfix release based on
> 4.0.1 + above patch, without going through the full release
> procedure...