Sharpie / RTikZDevice

A R package for producing graphics output as PGF/TikZ code for use in TeX documents.
32 stars 36 forks source link

R crashes during creation of temporary metrics dictionaries #16

Closed Sharpie closed 13 years ago

Sharpie commented 13 years ago

Reported in issue 15. Apparently only happens when Sweave is invoked from Rgui or Rscript.

Appears to be related to the creation of temporary metrics dictionaries:

5 : echo term verbatim tikz sanitize (label=mv_fig) 
Creating temporary TikZ metrics dictionary at:

    C:\DOCUME~1\user\LOCALS~1\Temp\Rtmp52vg3A/tikzMetricsDictionary

Error: no function to return from, jumping to top level
Execution halted

Files to reproduce located at:

http://www.sendspace.com/file/obbg5h

EDIT: Changed the name of the issue because it is not specific to Sweave

cameronbracken commented 13 years ago

So this is seriously strange, I currently have it narrowed down to something in the runme.R code.

require(tikzDevice)
tikz(sanitize=T)
source('runme.R')
# *crash*

It definitely has to do with the the sanitize option because it works fine without it. Can anyone else confirm this?

cameronbracken commented 13 years ago

Just got a new error:

Error in grid.Call.graphics("L_setviewport", pvp, TRUE) : 
  REAL() can only be applied to a 'numeric', not a 'NULL'
Sharpie commented 13 years ago

If I comment out the piece of my ~/.Rprofile that sets a permanent metrics dictionary, I get the following behavior for R 2.11.1:

The plot thickens.

Update

cameronbracken commented 13 years ago

#

Error in get(name, envir = asNamespace(pkg), inherits = FALSE)
    object 'sha1' not found
Error in dbExists(dictionary, sha1(key))
    error in evaluating the argument 'key' in selecting a method for function 'dbExists'

Error in grid.Call("L_setDLelt", x)
    SET_VECTOR_ELT() can only be applied to a 'list', not a 'NULL'
cameronbracken commented 13 years ago

I am now convinced that the problem has something to do with the way R is feeling when I open it. It is being moody lately.

Sharpie commented 13 years ago

What the hell did you feed it in the sanitize function? :P

Sharpie commented 13 years ago

I'm getting the same feeling you are-- R was running the example just fine this morning. Now it crashes in a different way.

It is really difficult to reproduce this problem and figure out exactly what is going on.

cameronbracken commented 13 years ago

It must have had a bad night

Sharpie commented 13 years ago

I'm starting to suspect a buffer overflow in the C implementation of Sanitize().

The reason is when buffer overflows occur, other pieces of memory get overwritten. This causes weird shit to happen such as random functions suddenly not being present in the symbol table. i.e. C symbol name "L_getEngineDLon" not in DLL for package "grid".

If I change the allocation on line 1572 of tikzDevice.c from:

char *cleanStringCP = (char *) calloc( strlen(cleanString), sizeof(char) );

To:

char *cleanStringCP = (char *) calloc( strlen(cleanString) * 2, sizeof(char) );

This essentially doubles the space available for the sanitized string and the crashes seem to disappear (at least for command line R, have not tried R GUI). I'm not advocating this as a final fix, but it could pin the problem down as being a buffer overflow, in which case we need to carefully consider our string lengths, allocation of string pointers, and freeing them when they are no longer used.

Could you check to see if this modification fixes the problem on your end?

Sharpie commented 13 years ago

Pretty positive that it is a string buffer overflow. According to devx.com, first Google hit for sizeof strlen:

Furthermore, there's a potential bug that may result from using strlen: it does not count the terminating null character. In order to get a string size correctly, the value returned from strlen has to be incremented by 1.

So the way calloc is used in Sanitize provides a variable that is 1 slot too small to hold the results of sanitizeTexString and so we are always causing a buffer overflow when this fucntion is called.

The tikzMetrics dictionary comes into the picture in the following way. When no permanent metrics dictionary is used a temporary one is created. The temporary contains no cached metrics, so R has to call TikZ_MetricInfo in addition to TikZ_Text. This doubles the number of calls to Sanitize therefore doubling the number of buffer overflows. The end result is that the probability something critical to R will be overwritten is increased when a temporary metrics dictionary is used.

Ain't C a beautiful language?

cameronbracken commented 13 years ago

Great! I Love C! I can confirm that the modification fixes the problem in the GUI and the command line. It makes sense that the problem is a buffer overflow. That would explain the erratic behavior. But this still surprises me since I thought that cleanStringCP would be allocated to whatever size the sanitized string was that came back from R. Do you know how to achieve this?

Just another example of me not really knowing wtf is going on in C and it coming back to bite us in the ass.

EDIT: So the line just needs to be

char *cleanStringCP = (char *) calloc( strlen(cleanString) + 1, sizeof(char) );
cameronbracken commented 13 years ago

Fixed:

http://github.com/cameronbracken/rtikzdevice/commit/8b354e01d62cd1cfede953b7b9e3ba93547217a7

Sharpie commented 13 years ago

Allright, I will pull it in and add some other stringlen-related fixes I have been meaning to do and then push to R-Forge.