fix: check CalloutNameEntry for NULL before access

[mhei]

This fixes a php segfault, see
openwrt/packages#12403

I traced it down with gdb
0xb6e37f70 in i_free_callout_name_entry (key=0x369040,
e=0x0, arg=0x37afe0) at oniguruma-6.9.5_rev1/src/regparse.c:1297

[kkos]

I have not been able to figure out where the problem is.
In openwrt/packages#12403

Program received signal SIGSEGV, Segmentation fault.
0xb6e37f70 in i_free_callout_name_entry (key=0x369040, e=0x0, arg=0x37afe0) at oniguruma-6.9.5_rev1/src/regparse.c:1297

But e is made so that it's not null, and arg shouldn't be passed as anything but zero.

Regardless, there's nothing wrong with this PR, so I accept it.

[cotequeiroz]

I'm not familiar with the code, but the comment about key->s being the same as e->name caught my attention. If e is NULL, shouldn't we check if key is not NULL, and free key->s to avoid a possible memory leak? We may still get an exception of key->s turns out to be bogus, but it may be an indication of a more serious problem somewhere else.

[kkos]

This issue is very strange to me.

(1) The segmentation fault occurs just by calling the termination process without using any other function of this library.
I hadn't changed the code around here for a long time.
If such a problem existed, it would have been reported long ago and I would have been aware of it myself before then.

(2) The values passed to function i_free_callout_name_entry are strange.
The argument 'e' is the value registered in the hash table, and if it's NULL, it shouldn't be registered. Therefore, it can't be passed as NULL.
The argument 'arg' is only called with the value 0, so it can't be called with a non-zero value.

The only place to call function i_free_callout_name_entry.

onig_st_foreach(t, i_free_callout_name_entry, 0);

The issue is a complete mystery, and I have no idea what to look for in it.

[mhei]

Thanks for digging into this. To be honest, I did not take the time to analyze the code around it, I just tried to setup the gdb environment to have a trace. I'm not that familiar with gdb, so please even consider that my backtrace is wrong. On the other side, with the fix/check for NULL applied, the original issue is gone...So yes, I'm confused to. What I can do is to rollback my test environment to the previous oniguruma version used in OpenWrt before and check whether the issue still - or better: already - occurs with this version...

[mhei]

My test environment is a I2SE Duckbill (arm mxs platform) with PHP 7.4.7:
a) oniguruma version 6.9.4, or to be more specific the oniguruma recipe from this commit: openwrt/packages@94895ec#diff-3bfdd4c918c5f5050eaaee6f9856c4b8 -> work without segfault
b) oniguruma version 6.9.5_rev1 + a backported cmake patch, or to be more specific this commit: openwrt/packages@fdc2394#diff-3bfdd4c918c5f5050eaaee6f9856c4b8 -> segfault
I just exchange the compiled library package.
Maybe this helps to track down the issue.

[kkos]

Thank you.
I'm limited by my lack of knowledge of OpenWrt, but I'll look into it as much as I can.

[mhei]

I think, you don't need any knowledge about OpenWrt. The summary is, that we used v6.9.4 and autotools without problems, and the library built with v6.9.5_rev1 + one additional patch using cmake build system does not work (anymore).

[cotequeiroz]

I think I found something. First of all, it works when autotools is used, but fails with cmake. The backport of the cmake versioning change I've written is not the problem. It fails even without the patch.
What I have found is that when building with cmake, warnings like this shows up:

src/regparse.c: In function 'onig_st_lookup_strend':
src/regparse.c:528:32: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
   return onig_st_lookup(table, (st_data_t )(&key), value);
                                ^

In function 'onig_st_insert_strend': result = onig_st_insert(table, (st_data_t )key, value);
In function 'onig_st_lookup_callout_name_table': return onig_st_lookup(table, (st_data_t )(&key), value);
In function 'st_insert_callout_name_table': result = onig_st_insert(table, (st_data_t )key, value);
In function 'onig_foreach_name': onig_st_foreach(t, i_renumber_name, (HashDataType )map);
In function 'name_add': onig_st_insert_strend(t, e->name, (e->name + (name_end - name)), (HashDataType )e);
In function 'callout_name_entry': r = st_insert_callout_name_table(t, enc, is_not_single, e->name, (e->name + (name_end - name)), (HashDataType )e);
In function 'setup_ext_callout_list_values': onig_st_foreach((CalloutTagTable *)ext->tag_table, i_callout_callout_list_set, (st_data_t )ext);

It could explain why the parameters had values that were not supposed to be. I'll look into it further when I get the time.

[cotequeiroz]

It did not take me that long. See #197

When building with cmake, SIZEOF_LONG_LONG and SIZEOF_VOIDP were both unset, which made them match, so st_data_t and hash_data_type were defined as long long instead of long. On x64, both of them are 8 bytes and everything works as expected. However, for arm, long is 4, long long is 8, and void* is 4, so you get the wrong type, and the parameters get messed up.

[kkos]

Thank you.
For a long time, the only thing I had to check about cmake was that it didn't give any errors on compilation.
But you're right, my environment is x64, so even if I had checked the execution, I wouldn't have found the problem.

[cotequeiroz]

It appears that cmake support is not really on par with autotools, is it? cmake builds it much faster, so I would prefer to use it, if all other things are equal. Do you have any recommendation?

[mhei]

This is great news that the cause was found! Thank you both @cotequeiroz and @kkos !

[kkos]

@cotequeiroz
Certainly no more tested in cmake than in autotools.
cmake is an alternative for those who cannot or do not want to use autotools.

@mhei
Thank you for your support.