by the way, i realised that i'm doing it wrong.

the original problem i was trying to solve was OOM when importing the whole SVN repo: Fossil worked up to 400+ revisions, and then it was killed. so i decided to use incremental imports instead.

the one thing i haven't realised is that i shouldn't pass "--incremental" to "svnrdump dump". it is not clear from the Fossil documentaion, and i have no expirience with SVN, so i thought that i should pass "--incremental" both to Fossil, and to svnrdump.

so i believe that help for "import --svn" command could be improved with a simple example, and with a mention that "--incremental" should only be used for Fossil.

it segfaults because incremental dump has no previous file, and binary svn diff wants to use the previous file. it may or may not work, depending of memory contents at pSrc. but even if it won't segfault, the resulting file will be invalid, because blob_size(pSrc) is zero.

i think that there should be a check at case 0x00: zCpy = blob_buffer(pSrc)+offSrc; break; -- check if pSrc is zero, and abort if it is. this way we will both avoid segfaulting, and catch invalid dump formats.

i think that there should be a check at case 0x00: zCpy = blob_buffer(pSrc)+offSrc; break; -- check if pSrc is zero, and abort if it is.

Thank you for the report and analysis.

i made that change but am still getting a segfault in another spot of that function (import.c:svn_apply_svndiff()) and don't currently understand the import code well enough to judge the circumstances under which that can happen. Perhaps someone closer to the import code can help us out here.

For completeness's sake:

Index: src/import.c
==================================================================
--- src/import.c
+++ src/import.c
@@ -1227,10 +1227,13 @@
   const char *zDiff = blob_buffer(pDiff);
   char *zOut;
   if( blob_size(pDiff)<4 || memcmp(zDiff, "SVN", 4)!=0 ){
     fossil_fatal("Invalid svndiff0 format");
   }
+  if( 0==pSrc ){
+    fossil_fatal("NULL input");
+  }
   zDiff += 4;
   blob_zero(pOut);
   while( zDiff<(blob_buffer(pDiff)+blob_size(pDiff)) ){
     u64 lenOut, lenInst, lenData, lenOld;
     const char *zInst;

then leads to:

(gdb) r
Starting program: /home/stephan/f/fossil/fossil import --svn --flat 00-bug.fossil 00_bug
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
svn_apply_svndiff (pDiff=pDiff@entry=0x7fffffffda48, pSrc=0x7fffffffde08, pSrc@entry=0x7fffffffde68, pOut=pOut@entry=0x7fffffffde68) at ./src/import.c:1271
1271	        *zOut++ = *zCpy++;
(gdb) bt
#0  svn_apply_svndiff (pDiff=pDiff@entry=0x7fffffffda48, pSrc=0x7fffffffde08, pSrc@entry=0x7fffffffde68, pOut=pOut@entry=0x7fffffffde68) at ./src/import.c:1271
#1  0x0000555555784c6a in svn_dump_import (pIn=<optimized out>) at ./src/import.c:1624
#2  import_cmd () at ./src/import.c:1915
#3  0x00005555557a55de in fossil_main (argc=<optimized out>, argv=0x7fffffffe068) at ./src/main.c:970
#4  0x00005555557a4da6 in main (argc=-8600, argv=0x0) at ./src/main.c:686

sorry, i haven't made myself clear.

pSrc is never NULL there: it is a valid blob, but of zero size. so we need to do something like: if (blob_size(pSrc) == 0) fossil_fatal("oops");.

and we need to do it exactly in that case branch, because empty source blob is perfectly valid if SVN diff doesn't contain "copy from source" opcode (in that case pSrc is never referenced at all).

... and we need to do it exactly in that case branch

Gotcha, thank you. That's now checked in. It doesn't solve the problem of being unable to handle empty input, but it does at least behave better than a NULL dereference.

thank you!

actually, i believe that error message could be changed to something like "do not use '--incremental' flag in svn dump command", or something along those lines.

this situation could happen only in two cases:

1. we have a corrupted SVN dump. rare case.

2. we are trying to process incremental SVN dump, and we don't have any previous SVN commit to get the original file data from.

"2" is exactly what i did wrong (by executing "svnrdump dump --incremental -r280:300"). here, revision 283 has delta applied to something which is stored before revision 280, and haven't written to the dump due to using "--incremental" flag.

so i think it is safe to assume that if we encountered zero-length pSrc and "copy from source" opcode, then the user is trying to do what i did. and we can tell the user how to fix the problem.

that error message could be changed to something like "do not use '--incremental' flag in svn dump command"

Very good. It now looks like:

Don't know how to handle NULL input. Tip: do not use the --incremental flag in the svn dump command.

Thank you for the report and suggestions.