[dev.icinga.com #2646] mismatched types for uint casts in bulkops (oracle)

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/2646

Created by gbeutner on 2012-05-25 08:32:56 +00:00

Assignee: (none)
Status: Closed (closed on 2013-09-21 17:39:06 +00:00)
Target Version: (none)
Last Update: 2014-12-08 14:37:43 +00:00 (in Redmine)

Icinga Version: 1.10.0
OS Version: any

---

The OCI_BindArrayOfUnsignedInts() function expects an array of uints (which are usually 32-bit). However the ido2db_handle_* functions incorrectly pass type-casted arrays of longs (which are usually 64-bit) to this function.

This causes the OCI_BindArrayOfUnsignedInts() function to interpret these 64-bit integers as 32-bit integers. Which in turn means that half of the rows that should be inserted are skipped and for every second row that actually is inserted 0s are used for all its columns (due to the fact that the MSBs of the 64-bit integer are all unset).

Here's a patch that should help identify the problem: https://gunnar-beutner.de/files/64-bit-compat-fixes.patch - I am currently unsure about whether OCI_BindArrayOfUnsignedInts is actually the right function; aren't these columns 64-bit? Which would mean that OCI_BindArrayOfUnsignedBigInts might be a suitable substitute there.

Changesets

2012-05-30 18:54:11 +00:00 by Tommi bc740127fbd09f3de4568e0de40ff2ed590ebe98

idoutils: (Oracle) fix wrong uint casts in bulk ops #2646
refs #2646

2012-06-21 08:57:09 +00:00 by Tommi 45c8f78

idoutils: fix mismatched types for uint casts in bulkops (oracle) #2646

refs #2646

[icinga-migration]

Updated by gbeutner on 2012-05-25 08:50:11 +00:00

Actually, the OCI_BindArrayOfUnsignedInts()s aren't the only problem. The same thing happens with other binds:

void *data[4]; <- ok, that's pretty horrible in itself, but lets ignore that for now

data[0] = (void ) &idi~~dbinfo.instance_id; <~~ alright, instance_id is a long, so data[0] is actually a pointer to long (long).

if (!OCI_BindUnsignedInt(idi->dbinfo.oci_statement_objects_select_name1_name2, MT (":X1"), (uint ) data[0])) { <- which we now cast to a uint, which is just incorrect.

However, this mostly works. Mostly because there's an unintended dependency on the memory layout of longs and ints. That could would not work correctly on big endian machines. And even on little endian machines it will not work correctly when instance_id is larger than UINT_MAX (ok, instance_ids might be a bad example for that seeing how nobody ever has more than UINT_MAX instances, but this problems also affects other object ids).

[icinga-migration]

Updated by Tommi on 2012-05-27 07:48:29 +00:00

• Status changed from New to Assigned
• Assigned to set to Tommi

Is this a theoretical or a practical issue? Did you got wrong IDs in your database?

Some constructs in the code are not best practice as written in books, this also applies for some casts which match for 32bit, but are not really proper in 64bit. In a 32bit system unsigned long and ocilib type uint are the same size, thats the way it was developed. In 64bit ulong is 8Byte, but the ocilib function still requires 4Byte. The code was not fully rewritten, most time only the casts are included to prevend SEGV in 32bit and is safe for IDs in 32bit range (i never saw more). To be fully compliant all id variables must be rewritten as unsigned long long and the function changed to BindArrayOfUnsignedBigInts(). Thats a bigger story.

BTW: what about your datatype issue #2362

[icinga-migration]

Updated by gbeutner on 2012-05-29 06:30:24 +00:00

A colleague of mine has seen the first issue (i.e. OCI_BindArrayOfUnsignedInts) in production use.

The second issue is somewhat theoretical - so far I haven't actually encountered anyone who's running Icinga on a big endian system.

[icinga-migration]

Updated by Tommi on 2012-05-29 09:39:12 +00:00

what for errors appeared on the system of your collegue? Missing entries, wrong IDs or anything else...?
Usually the cast is aware of the byte order. But for ocilib i am not sure. On my solaris systems i never saw any problems.

[icinga-migration]

Updated by gbeutner on 2012-05-29 11:50:11 +00:00

This causes the OCI_BindArrayOfUnsignedInts() function to interpret these 64-bit integers as 32-bit integers. Which in turn means that half of the rows that should be inserted are skipped and for every second row that actually is inserted 0s are used for all its columns (due to the fact that the MSBs of the 64-bit integer are all unset).

This pretty much describes the problem my colleague was seeing. Please let me know if you need more details.

[icinga-migration]

Updated by Tommi on 2012-05-30 19:05:05 +00:00

• Status changed from Assigned to Feedback
• Done % changed from 0 to 90

first fix in changeset bc740127fbd09f3de4568e0de40ff2ed590ebe98 targeting all bulk ops(ArrayOf-calls). In my test system(linux) IDs are now ascending without any gaps. Pls try out

[icinga-migration]

Updated by mfriedrich on 2012-06-21 08:56:29 +00:00

• Subject changed from Mismatched types for OCI_BindArrayOfUnsignedInts to mismatched types for uint casts in bulkops (oracle)
• Target Version set to 1.8

[icinga-migration]

Updated by jmosshammer on 2012-07-06 12:25:32 +00:00

We had the issue on a customer db today. Tommi's commit didn't quite work and caused results like this:
ID INSTANCE_ID HOSTGROUP_ID HOST_OBJECT_ID

---

1 1 1 32
2 1 1 33
3 1 2 1
4 1 2 3
5 20173824 0 0
6 20173824 0 0
7 20174144 0 0
8 0 0 0
9 0 0 0
10 161 0 0
11 20174256 0 0

ID INSTANCE_ID HOSTGROUP_ID HOST_OBJECT_ID

---

12 20283296 0 0
13 0 0 0
14 17512160 0 0
15 0 0 0
16 0 0 0
17 0 0 0
18 1 3 27
19 1 4 17
20 2,6852E+11 2,6852E+11 2,6852E+11
21 22284976 20173824 0
22 22284976 20173824 0

ID INSTANCE_ID HOSTGROUP_ID HOST_OBJECT_ID

---

23 0 1 2
24 0 1 2
25 0 1 2
26 0 1 2
27 0 1 2
28 0 1 2
29 0 1 2
30 1 7 28
31 1 7 29
32 1 7 30
33 1 7 31

Using only Gunnar's patch seemed to fix the problem.

[icinga-migration]

Updated by Tommi on 2012-07-06 14:09:21 +00:00

are you sure you included the fix?
I did exactly this what gunnar suggested, removed the casts and replaced with real big_uints. Maybe you can point out, where i am wrong

big_uint  *instid_arr;
big_uint  *groupid_arr;
big_uint  *memberid_arr;
...
instid_arr = (big_uint *) malloc(OCI_BINDARRAY_MAX_SIZE * sizeof(big_uint));
groupid_arr = (big_uint *) malloc(OCI_BINDARRAY_MAX_SIZE * sizeof(big_uint));
memberid_arr = (big_uint *) malloc(OCI_BINDARRAY_MAX_SIZE * sizeof(big_uint));

OCI_BindArrayOfUnsignedBigInts(idi->dbinfo.oci_statement_hostgroupdefinition_hostgroupmembers, MT(":X1"), instid_arr, 0); 
 CI_BindArrayOfUnsignedBigInts(idi->dbinfo.oci_statement_hostgroupdefinition_hostgroupmembers, MT(":X2"), groupid_arr, 0); 
OCI_BindArrayOfUnsignedBigInts(idi->dbinfo.oci_statement_hostgroupdefinition_hostgroupmembers, MT(":X3"), memberid_arr, 0);
 ...
/* copy ids to to array */
instid_arr[arrsize] = idi->dbinfo.instance_id;
groupid_arr[arrsize] = group_id;
memberid_arr[arrsize] = member_id;
arrsize++;

[icinga-migration]

Updated by mfriedrich on 2012-07-06 17:14:21 +00:00

hmm gunnar's patch replaces unsigned long's with unsigned int's ...

-   unsigned long  *instid_arr;
-   unsigned long  *fileid_arr;
+   unsigned int *instid_arr;
+   unsigned int *fileid_arr;

the patch on git uses big_uint's instead

-   unsigned long  *instid_arr;     
-   unsigned long  *fileid_arr; 
+   big_uint  *instid_arr;  
+   big_uint  *fileid_arr;

since big_uint is an ocilib internal specification, see ocilib.h

/**
 * @typedef big_int
 *
 * @brief
 * big_int is a C scalar integer (32 or 64 bits) depending on compiler support for 64bits integers.
 * big_uint is an usigned big_int
 *
 */

/* check for long long support */

#if defined(_LONGLONG) || defined(LONG_LONG_MAX) || defined(LLONG_MAX) || defined(__LONG_LONG_MAX__)

/* C99 long long supported */

typedef long long big_int;
typedef unsigned long long big_uint;

  #define OCI_BIG_UINT_ENABLED

#elif defined(_WINDOWS)

/* Microsoft extension supported */

typedef __int64 big_int;
typedef unsigned __int64 big_uint;

  #define OCI_BIG_UINT_ENABLED

#else

typedef int big_int;
typedef unsigned int big_uint;

#endif

an unsigned long long is slightly different to unsigned int in that case. question remains, who's right now.

OCI_BindArrayOfUnsignedInts was the initial binding functionality, where the unsigned int matches perfectly.
the patch on git changes that to OCI_BindArrayOfUnsignedBigInts which introduces a new set of Unsigned*Big*Ints instead of UnsignedInts.

imho the casts on big_uint do not fit on the given values. instance_id as well configfile_id are both unsigned long's where an unsigned long long would probably cause irritations. i do remember a patch by Carl or someone else, revoking that regression on the current code in a similar manner.

[icinga-migration]

Updated by Tommi on 2012-07-06 17:49:43 +00:00

Gunnars ID to use the same datatype is OK, but we need the 64bit version of INTs to handle IDs >2Mio. Long is 32bit on 32bit systemes but 64bit in some 64bit systemes therefore the initial issue happens when casting a 64bit long into the (always) 32bit uints.
With the patch should be ensured by ocilib to match always the right datatypes when declaring the array as big_uint * and passing the array via ocilib to oracle with OCI_BindArrayOfUnsignedBigInts. I know the data like instance_id is typed unsigned long, but i was asuming a copy of the value into the array will do the right things with the value when long on the system is only 32bit

instid_arr[arrsize] = idi->dbinfo.instance_id;

Additional i would like to check if the patch is really in place because the latest 1.7.x Releases doesnt have it and if ocilib is >=3.9.2. This is importantent because previous version sometimes choosed the wrong type for big_ints. One of the conditions was missed here. I had this discussion with Vincent about

#if defined(_LONGLONG) || defined(LONG_LONG_MAX) || defined(LLONG_MAX) || defined(__LONG_LONG_MAX__)

[icinga-migration]

Updated by mfriedrich on 2012-09-03 17:41:07 +00:00

• Assigned to changed from Tommi to mfriedrich
• Target Version deleted 1.8
• Icinga Version set to 1
• (unknown custom field) set to 1
• OS Version set to 64
• (unknown custom field) set to Oracle
• (unknown custom field) set to 11

i guess we should have some private personal chat on that thingy. i'll unschedule it for 1.8 now, but we keep an eye on it, and put it as todo with further tests together.

[icinga-migration]

Updated by mfriedrich on 2013-09-21 17:39:06 +00:00

• Status changed from Feedback to Closed
• Assigned to deleted mfriedrich

[icinga-migration]

Updated by mfriedrich on 2014-12-08 14:37:44 +00:00

• Project changed from 18 to Core, Classic UI, IDOUtils
• Category changed from 79 to IDOUtils
• Icinga Version changed from 1 to 1
• OS Version set to any