[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

There still are places, which access Mem.u.i/u without
checking flags properly. Please, for each place either
explain why it is correct, or fix it and add a test.

--------------------------------------------------------------
vdbe.c:2659:
	if ((pDest->flags & (MEM_Int | MEM_UInt)) != 0) {
		if (field_type == FIELD_TYPE_NUMBER)
			sqlVdbeMemSetDouble(pDest, pDest->u.i);
	}

You can't access pDest->u.i - it can be a big unsigned.

--------------------------------------------------------------
vdbe.c:2955:
	pIn1 = &aMem[pOp->p1];
	uint32_t space_id = pIn1->u.i;

Here you touch u.i, but it is always
unsigned. You should use u.u, I think.

--------------------------------------------------------------
vdbeaux.c:2428:
	if (flags & MEM_Int) {

		...

		i64 i = pMem->u.i;
		u64 u;
		if (i < 0) {
			u = ~i;
		} else {
			u = i;
		}

Why do you check 'i < 0', if you already
see its flag is 'Int'. It should be < 0 by
definition.

--------------------------------------------------------------
vdbeaux.c:2589:
		u64 v;
		u32 i;
		if (serial_type == 7) {
			assert(sizeof(v) == sizeof(pMem->u.r));
			memcpy(&v, &pMem->u.r, sizeof(v));
			swapMixedEndianFloat(v);
		} else {
			v = pMem->u.i;
		}

Why are you sure it is safe to store u.i into
uint64_t variable?

--------------------------------------------------------------
vdbeaux.c:2644:
	u64 x = FOUR_BYTE_UINT(buf);
	u32 y = FOUR_BYTE_UINT(buf + 4);
	x = (x << 32) + y;
	if (serial_type == 6) {
		/* EVIDENCE-OF: R-29851-52272 Value is a big-endian 64-bit
		 * twos-complement integer.
		 */
		pMem->u.i = *(i64 *) & x;
		pMem->flags = MEM_Int;

Vice versa - why is it safe to store uint64_t into u.i,
and set its flag to 'Int' (== value is negative).

Actually, all this 'serial' shit looks broken. Please,
verify that code.

--------------------------------------------------------------
vdbeaux.c:2998:
		if ((f1 & MEM_UInt) != 0) {
			if ((f2 & MEM_Real) != 0) {
				return sqlIntFloatCompare(pMem1->u.i,

pMem1 is unsigned, according to the first check,
but you use u.i. Why?

--------------------------------------------------------------
vdbeaux.c:3184:
 do_int:
			if ((pKey2->flags & (MEM_Int | MEM_UInt)) != 0) {
				if (mem1.u.i < pKey2->u.i) {
					rc = -1;

pKey2 is an integer, but you don't know the sign. And use u.i,
assuming it is negative, or at least small enough. Why?

Line 3207 is the same.

--------------------------------------------------------------
vdbemem.c:1431:
			} else if (pVal->u.i == SMALLEST_INT64) {
				pVal->u.r = -(double)SMALLEST_INT64;
				MemSetTypeFlag(pVal, MEM_Real);
			} else {
				pVal->u.i = -pVal->u.i;
			}

You compare u.i and SMALLEST_INT64, but you can't
be sure, that u.i is not a big unsigned, can you?

--------------------------------------------------------------
I will verify u.u, MEM_Int, and MEM_UInt usages, when we
finish with u.i.