iuke-tier@ey m@iii@g oii uiow@@edu
2020-Jun-07 22:03 UTC
[Rd] [External] Re: 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
peter dalgaard
2020-Jun-07 23:00 UTC
[Rd] [External] use of the tcltk package crashes R 4.0.1 for Windows
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
Avraham Adler
2020-Jun-07 23:28 UTC
[Rd] [External] use of the tcltk package crashes R 4.0.1 for Windows
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]]
Jeroen Ooms
2020-Jun-08 07:36 UTC
[Rd] [External] Re: use of the tcltk package crashes R 4.0.1 for Windows
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...
Fox, John
2020-Jun-08 13:45 UTC
[Rd] [External] use of the tcltk package crashes R 4.0.1 for Windows
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...