Repository navigation
fread segfaults when reading a file with size a multiple of 4k #2194
Description
Activity
This requires a much larger change than I originally anticipated, so here's the outline:
-
The idea of writing
\nto the last byte of the memory-mapped region, although neat, sometimes fails to work, and so will be removed. Instead, we'll use the following strategy: search the file from the end for the last newline, and then move the last line (appending a newline) into a separate memory buffer. This splits the entire file into 2 separate parts: "main" partsof .. eof, and "hidden" partsoh .. eoh. For example, if the input wasA,B\n1,2\n3,4, then the first part will becomeA,b\n1,2\n, and the second3,4\n. Both parts will have valid newline endings. -
The iteration over the file will have to be organized more carefully: while previously it was as simple as
const char *ch = sof; while ((ch < eof) && <condition>) ...;Now it becomes more complicated:
const char *ch = sof, *end = eof; while (((ch < end) || (eoh && (end != eoh) && (ch=soh) && (end=eoh)) && <condition>) ...;fortunately, this doesn't add any runtime overhead (assuming compiler does proper short-circuit evaluation).
-
All parser functions will no longer require any
ch < eoftests: the test*ch == eolis sufficient to test the end of the field. Theeofvariable is even no longer global (it can't be: different threads might be scanning different portions of the file simultaneously). This change is the original reason why we are going to such trouble to ensure there's always aneolat the end file. There is only 1 problem here: quoted string fields with embedded newlines.In order to circumvent this difficulty, we change the semantics of the
_Bool Field(const char **ptr, void *target)function (currently this function parses the next string field and returns true/false indicator of success/failure; on success it also advances the pointerptrto the end of the field just read). In the new semantics this function hasintreturn value, 0 meaning success, 1 failure, and 2 "continuation failure". The latter status is returned for fields that are correctly quoted but the parser hit a newline before the field ended (thus, theField()parser will stop parsing at a newline like all other parsers do). When status 2 is returned, the parser will also fill out thelenOff targetstructure with detected offset and length until the newline, and it will advance the pointerptr.Additionally, there will be a second field parsing function
int continue_parse_string(const char **ptr, lenOff *target), which will (a) assume that it begins inside a quoted field, (b) in case of successful scan incrementstarget->offinstead of overwriting it. This function also returns status 0 for success (field scanned till the end), 1 for failure (field is invalid under current quoting rule, for example quote is not followed by a field separator), and 2 for "continuation failure" (i.e. the field didn't end on this line: keep scanning). Both statuses 0 and 2 advance pointerptrand incrementtarget->off. -
Now the field parser logic will have to become more complicated as well: if previously it was
while (!fun[type[j]](&ch, target[j])) { type[j]++; ch = fieldStart; }Now it is more like this:
while (ret = fun[type[j]](&ch, target[j])) { while (ret == 2) { ch += eolLen; if (ch == end) { if (end == eof) { ch = soh; end = eoh; } else { ret = 1; break; } } ret = continue_parse_string(&ch, target[j]); } if (ret == 1) { type[j]++; ch = fieldStart; } // if `ret` is 0 then don't do anything: field was parsed correctly }This adds small amount of overhead, but only for multi-line string fields, which are sufficiently rare. Overall runtime impact should be negligible for the most common cases.
-
There are also lots of minor code changes: added more messages in verbose mode (and some of the existing ones may have been tweaked); added
ASSERT(cond)macro that expands intoif (!cond) STOP("Internal error on line %d, please report", __LINE__)-- this is intended for checks that should in theory never fail (and thus shouldn't show up red on the coverage report); theposvariable was removed, instead thesofvariable is used to track the beginning of data section (it would be too hard to jugglepos/sof/sohtriple); comments are added in various places to clarify the meaning of the new / existing code; in some cases compound statements are split into multiple lines to get a more accurate code coverage metric; moved around some functions so that there are 3 distinct sections: all helper functions first, then all parsers, then the mainfreadMain.
(the PR itself is not ready yet, but this is the plan of attack)
-
- changed the title
[-]fread segfaults on MacOS when reading a file whose size is a multiple of 4k[/-][+]fread segfaults when reading a file with size a multiple of 4k[/+]on Jul 4, 2017 - added this to the milestone
on Aug 4, 2017 Hi all,
I'm sorry but I may have to ask a stupid question: This issue should already be fixed in the current release of data.table on CRAN, should it not? I'm running on what I think is a very similar error, however I don't want to alert anyone if this is not yet implemented in the current version of the package.
Thanks and best!
johann@st-pasha did this make it into
1.10.4-3?Is there a quick fix to this issue while we wait 1.10.6, as this is really problematic for me. I bulk fread a few thousand dynamically generated csv files and occasionally one will crash the process. tryCatch doesn't work.
@httassadar I believe the current dev version does not segfault when the file is an exact multiple of 4096 bytes (though you might encounter an error, with instructions for a workaround). Install instructions here: https://github.com/Rdatatable/data.table/wiki/Installation
I am still getting this issue using version 1.12.8? Any ideas why?
@jngranger are you able to reproduce it when trying to read file attached in the first post? if not, then are you able to share your files? if not, please provide
verbose=TRUEoutput offread.I attached the file I was trying to read here. The code I used to read it in was : fread(FileExample, header=F,skip=23,fill=T)
Is that file a multiple of 4096 bytes? On my computer it isn't.
@jngranger you may want to open new issue, it might be easier for us to follow up than here. Especially if file is not a multiple of 4096 bytes. Thank you.
Happens in line 768:
*_const_cast(eof) = eol;The error is:
4k.txt