VFK driver : memory leak and memory use errors

[Even Rouault]

Martin,

I'm attaching the output of "valgrind --leak-check=full ogrinfo -ro -al data/bylany.vfk". It shows a memory leak and memory use errors

[Even Rouault]

Valgrind output

[Even Rouault]

Martin, the Conditional jump or move depends on uninitialised value(s) and Invalid write of size 1 errors are fixed by :

Index: ogr/ogrsf_frmts/vfk/vfkreader.cpp
===================================================================
--- ogr/ogrsf_frmts/vfk/vfkreader.cpp	(révision 18468)
+++ ogr/ogrsf_frmts/vfk/vfkreader.cpp	(copie de travail)
@@ -357,7 +357,7 @@
 	poChar++;
     }
     pszValue = (char *) CPLMalloc(iValueLength + 1);
-    strncpy(pszValue, poValue, iKeyLength);
+    strncpy(pszValue, poValue, iValueLength);
     pszValue[iValueLength] = '\0';
 
     poInfo[pszKey] = pszValue;

I let you commit this fix in case you are just working on it ?

Looking at that function, I think there might be issues if there are no ';' or '\r' character in the string you're parsing. But that's more about robustness against invalid data issue. For later, I'd say

The interesting thing is that after your first memory leak fixes, new ones have been unveiled (they were seen as 'possibly lost', but are now confirmed as 'definitely'). See log2.txt attached

[Even Rouault]

Valgrind log after r18468 + my uncommited patch proposal

[martinl]

Please try r18469

==12539== LEAK SUMMARY:
==12539==    definitely lost: 0 bytes in 0 blocks
==12539==    indirectly lost: 0 bytes in 0 blocks
==12539==      possibly lost: 0 bytes in 0 blocks
==12539==    still reachable: 12 bytes in 3 blocks
==12539==         suppressed: 0 bytes in 0 blocks

[martinl]

Replying to rouault:

Martin, the Conditional jump or move depends on uninitialised value(s) and Invalid write of size 1 errors are fixed by :

> Index: ogr/ogrsf_frmts/vfk/vfkreader.cpp
> ===================================================================
> --- ogr/ogrsf_frmts/vfk/vfkreader.cpp	(révision 18468)
> +++ ogr/ogrsf_frmts/vfk/vfkreader.cpp	(copie de travail)
> @@ -357,7 +357,7 @@
>  	poChar++;
>      }
>      pszValue = (char *) CPLMalloc(iValueLength + 1);
> -    strncpy(pszValue, poValue, iKeyLength);
> +    strncpy(pszValue, poValue, iValueLength);
>      pszValue[iValueLength] = '\0';
>  
>      poInfo[pszKey] = pszValue;

I let you commit this fix in case you are just working on it ?

Right, thanks, committed in r18470. Martin

[Even Rouault]

Martin,

yep with r18470, valgrind is now happy. Thanks for being so reactive. I'm closing the ticket.

Just stylistic node : you don't need to test if foo is NULL to call VSIFree() or CPLFree(). The same applies for the C++ delete operator actually.

When you want to have some live discussions with some of us, you know you can generally join us on IRC : #gdal on freenode.net. That's sometimes easier in phases like that one, integration of new stuff.