Surface Matching and x64 : heap corruption

[rbregier]

Hi,

In surface_matching\src\ppf_match_3d.cpp a function "murmurHash" is used like the following :
KeyType hashKey=0;
murmurHash(key, 4*sizeof(int), 42, &hashKey);

In case of a 64bit system, "murmurHash" is defined via a macro as "hashMurmurx64" in surface_matching\src\hash_murmur.hpp
However this function takes a void* as a parameter which should at least of length (2*sizeof(unsigned int)). Here &hashKey is given, and hashKey is an unsigned int, thus there is a heap corruption.

A workaround consists in forcing "murmurHash" to be defined as hashMurmurx86 even on 32 bit system, but in this case I don't understand the purpose of specific code for x64 systems.

Regards,
Romain

[ahundt]

do you have a patch for this?

[bmagyar]

Those 2 methods are intentional. If they are mixed up, with one alternative the precision of the surface matching drops dramatically, with the other it takes a lot of time.

[mshabunin]

@rbregier , please reopen this issue if you have reproducing example.

[rbregier]

Well, the example program crashes with the example dataset during training:
ppf_load_match.exe parasaurolophus_6700.ply rs1_normals.ply

In debug mode, Visual Studio throws an exception :

In ppf_match_3d.cpp :

And my investigations lead me to the conclusion of my first message.

When asking to Tolga Birdal (the named author in the source code) about it in February, he answered me the following :
"A new version will come out soon hopefully fixing many of the bugs. However, we have tried it on x64 systems, and verified identical results.
As a workaround, you could use hashMurmurx86 on both systems."

My system settings :
Windows 7 64bits
Compiled with Visual Studio 2013 as an x64 executable

So, yes I can provide the fix proposed in my first message, but the resulting performances were not awesome if I remember correctly and according to @bmagyar this fix is suboptimal.
@bmagyar do you have a better solution?

[bmagyar]

At the time of Tolga's GSoC project, I tested this on all platforms I had available: Ubuntu 32bit, Ubuntu 64bit, OSX 64bit. He was using Windows so it is strange that this didn't come up as well as that no warning about this is reported by buildbot (!).

Frankly, I didn't like that macro magic there in the first place, but at the time that was the best shot. This could be solved by using a hashing library that does the 32/64 switching internally, but I'm also curious to see what Tolga is coming up with.

[bmagyar]

@tolgabirdal Do you have any thoughts on this?

[tolgabirdal]

This might be due to modification made later. Let us define KeyType as follows:

#if (defined x86_64 || defined _M_X64)
typedef uint64_t KeyType;
#else
typedef unsigned int KeyType;
#endif

I will be submitting a pull request with that.

[rbregier]

I can confirm that with this definition it runs :

#if (defined x86_64 || defined _M_X64)
#include <stdint.h>
typedef uint64_t KeyType;
#else
typedef unsigned int KeyType;
#endif

However, I get some warnings regarding casts from KeyType to int, unsigned int and size_t. I don't know the effects of those on the algorithm.

[bmagyar]

How about using KeyType in favor of int in those instances? Could you try that please?

[rbregier]

I think it would require a more consequent code refactoring as a default hash function using "unsigned int" (called hash) is used in several place in the code such as for ICP to build hashtables and it is unclear what should be the key type for thoses, but I don't have time to dive into the code and try to figure out how it works in detail. I guess Tolga could fix this easily in his pull request.

[LaurentBerger]

In this question same problem exist in opencv-3.2-dev. I replace code as suggested by @tolgabirdal and it works. May be a PR is necessary.

[tolgabirdal]

I do have it:
#811
But for now seems to have some unsuccessful builds and should be fixed.

[LaurentBerger]

@tolgabirdal I have got VS 2013. there is no definition for uint64_t in VS2013 (that's OK in vs2015)
you can try :
#if (defined x86_64 || defined _M_X64)
#if _MSC_VER <= 1800
typedef __int64 int64_t;
typedef unsigned __int64 uint64_t;
#endif
typedef uint64_t KeyType;

#else
typedef unsigned int KeyType;
#endif

you will have some warnings :
1>G:\Lib\opencv_contrib\modules\surface_matching\src\t_hash_int.cpp(122): warning C4244: 'argument' : conversion de 'cv::ppf_match_3d::KeyType' en 'unsigned int', perte possible de données
1>G:\Lib\opencv_contrib\modules\surface_matching\src\t_hash_int.cpp(189): warning C4244: 'argument' : conversion de 'cv::ppf_match_3d::KeyType' en 'unsigned int', perte possible de données
1>G:\Lib\opencv_contrib\modules\surface_matching\src\t_hash_int.cpp(214): warning C4244: 'argument' : conversion de 'cv::ppf_match_3d::KeyType' en 'unsigned int', perte possible de données
1>G:\Lib\opencv_contrib\modules\surface_matching\src\ppf_match_3d.cpp(293): warning C4244: '=' : conversion de 'cv::ppf_match_3d::KeyType' en 'int', perte possible de données

[apassenger]

I am using VS 2013 (64 bit)I tried this:

#if (defined x86_64 || defined _M_X64)
#if _MSC_VER <= 1800
typedef __int64 int64_t;
typedef unsigned __int64 uint64_t;
#endif
typedef uint64_t KeyType;

#else
typedef unsigned int KeyType;
#endif

Unfortunately this could not my problem (My problem : Stack around the variable 'hashKey' was corrupted.)

@tolgabirdal @LaurentBerger

[wangsen1312]

@apassenger Did you solve the problem? I have encountered the same problem? still not solved.
My configuration is VS 2013 windows

[opencvbel]

I am using VS 2015 (64 bit) on a Windows 10 machine with OpenCv 3.2.0 .
I made the changes suggested by @tolgabirdal in the file t_hash_int.hpp where KeyType was defined,
but same runtime error as before which is 'Run-Time Check Failure # 2 - Stack around the variable 'hashKey' was corrupted'.
Any ideas?

[feliwir]

Any updates on this? I am using the 3.2 version and get this error with VS2013 x64 aswell

[JoyMazumder]

I have added that but the still same problem. openCV 4.0.0, VS -2015
I also tried at opencv 3.4.2

[JoyMazumder]

@opencvbel , @apassenger Did you solve that problem?

[asjgit]

this problem happend at debug model
at release model processing right， but wrong result

[middlestone]

is there any progress on this issue? i meet it using VS2017 and openCV 4.0.0 too.

[pclbel]

@JoyMazumder No couldnt find any solution, sorry

[tolgabirdal]

Guys, I have a PR under: #811 This doesn't pass all the tests yet and I do not have the time right now to update. However, that could hopefully solve this issue. Cheers,

…

On Thu, Feb 14, 2019 at 12:18 PM pclbel ***@***.***> wrote: @JoyMazumder <https://github.com/JoyMazumder> No couldnt find any solution, sorry — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#170 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGqlvfGAPoB0rSDWn9ZufM7528zDAWNEks5vNUYigaJpZM4Dfe5J> .

-- /tolga

[YYuan-pcl]

I can confirm that with this definition it runs :

#if (defined x86_64 || defined _M_X64)
#include <stdint.h>
typedef uint64_t KeyType;
#else
typedef unsigned int KeyType;
#endif

However, I get some warnings regarding casts from KeyType to int, unsigned int and size_t. I don't know the effects of those on the algorithm.

It works！However，the performance of the algrithm （after using the changes above）now is quite poor. We cannot even match the samples given in the opencv well.
Very frustrated。。。

[YYuan-pcl]

In this question same problem exist in opencv-3.2-dev. I replace code as suggested by @tolgabirdal and it works. May be a PR is necessary.

It works！However，the performance of the algrithm （after using the changes above）now is quite poor.

[YYuan-pcl]

this problem happend at debug model
at release model processing right， but wrong result

It works！However，the performance of the algrithm （after using the changes above）now is quite poor. do you have ideas

[luck-boy1994]

any one solve this problem?
Run-Time Check Failure #2 - Stack around the variable 'hashKey' was corrupted.

static KeyType hashPPF(const Vec4d& f, const double AngleStep, const double DistanceStep)
{
Vec4i key(
(int)(f[0] / AngleStep),
(int)(f[1] / AngleStep),
(int)(f[2] / AngleStep),
(int)(f[3] / DistanceStep));
KeyType hashKey = 0;

murmurHash(key.val, 4*sizeof(int), 42, &hashKey);
return hashKey;
}
opencv_4.0.0 x64 vs2017

[alalek]

@luck-boy1994 You using outdated version.

Issue is fixed here: #2347

[luck-boy1994]

@ luck-boy1994您使用的是过时的版本。

问题已在此处修复：＃2347

thank you

[YYuan-pcl]

Excuseme ,opencv with new version doesn't have this problem,right?&nbsp; for example ,opencv 4.0 doesn't have this problem

…

------------------&nbsp;原始邮件&nbsp;------------------ 发件人:&nbsp;"luck-boy1994"<notifications@github.com&gt;; 发送时间:&nbsp;2020年4月19日(星期天) 晚上9:22 收件人:&nbsp;"opencv/opencv_contrib"<opencv_contrib@noreply.github.com&gt;; 抄送:&nbsp;"823473780"<823473780@qq.com&gt;;"Comment"<comment@noreply.github.com&gt;; 主题:&nbsp;Re: [opencv/opencv_contrib] Surface Matching and x64 : heap corruption (#170) @ luck-boy1994您使用的是过时的版本。 问题已在此处修复：＃2347 thank you — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.