check idea: warn when mixing signedness in ?: operator (got bitten by this recently)

Hi,

If the <then> and <else> expression in the ?: ternary operator have different signedness they will both be implicitely casted to unsigned.

When the result is stored in a variable with a storage capable of holding both values, this is very unexpected. Consider this example:

    int rc = -1;
    unsigned int foo = 123;
    long x = y ? foo : rc;

If one of the branch of the ?: is unsigned, then the compiler will cast both branch to unsigned _before_ storing it in x. Despite long being able to store INT_MIN, INT_MAX, UINT_MAX (assuming 64b long/32b int).

So if y is 0, it's basically doing

    long x = (long)((unsigned int)-1);

Which will result in storing 0x00000000ffffffff (4294967295) instead of expected 0xffffffffffffffff (-1).

I thought we hit some sort of weird compiler bug but after reducing the problem to the simple example above and trying it GCC, clang, ICC and MSVC they all do the same thing: https://godbolt.org/z/P5Ts7o1df

So it is most likely a C quirk. Standard reads 6.5.15. 5)

Cheers,

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Hmmm,
I'm wondering what you would be warned about:
- about 'y ? foo : rc' becoming unsigned?
- about the cast 'unsigned int' -> '(signed) long' doing an zero-extension and not a sign extension?

In both cases, it's not very different than:

	int rc = -1;
	unsigned int foo = 0;
	long x = foo + rc;

and it boils down to the difference between:

	long x = (long)((unsigned int)-1);
and
	long x = (long)((int)-1);

Yes, it' also what Sparse is doing.

Cheers,
-- Luc

Thanks for the idea. I can implement something like that in Smatch. I'll run the attached check over the kernel and see what it turns up.

It says that it's only checking assignments but the trick is that Smatch creates fake assignments in the background for passing parameters or returning. So "return a ? uint_val : -ENOMEM;" will trigger an error message.

If there are too many false positives when I test this tonight, then I may make is_suspicious_int() more strict.

regards,
dan carpenter

/*
* Copyright (C) 2021 Oracle.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
*/

/*
* Type promotion with selects doesn't work how you might expect:
* long foo = bar ? u_int_type : -12;
* The -12 is promoted to unsigned int and the sign is not expanded. *
*/

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

static bool is_select(struct expression *expr)

{
	if (expr->type == EXPR_CONDITIONAL)
		return true;
	if (expr->type == EXPR_SELECT)
		return true;
	return false;
}

static int is_uint(struct expression *expr)

{
	struct symbol *type;

	type = get_type(expr);
	if (type_positive_bits(type) == 32)
		return true;
	return false;
}

static int is_suspicious_int(struct expression *expr)

{
	struct symbol *type;
	struct range_list *rl;

	type = get_type(expr);
	if (type_positive_bits(type) != 31)
		return false;

	get_absolute_rl(expr, &rl);
	if (!sval_is_negative(rl_min(rl)))
		return false;

	return true;
}

static void match_assign(struct expression *expr)

{
	struct expression *right, *one, *two;
	struct symbol *type;
	char *name;

	if (expr->op != '=')
		return;

	right = strip_expr(expr->right);
	if (!is_select(right))
		return;

	type = get_type(expr->left);
	if (type_bits(type) != 64)
		return;

	if (right->cond_true)
		one = right->cond_true;
	else
		one = right->conditional;
	two = right->cond_false;

	if (is_uint(one) && is_suspicious_int(two)) {
		name = expr_to_str(two);
		sm_warning("check sign expansion for '%s'", name);
		free_string(name);
		return;
	}

	if (is_uint(two) && is_suspicious_int(one)) {
		name = expr_to_str(one);
		sm_warning("check sign expansion for '%s'", name);
		free_string(name);
		return;
	}
}

void check_select_type(int id)

{
	my_id = id;
	add_hook(match_assign, RAW_ASSIGNMENT_HOOK);
}

Hi Dan,

I've only used sparse I think (make C=1) I need to lookup how to use Smatch.

Sounds good.

If that's any help, the exact bug where we hit this is currently in fs/cifs/file.c in collect_uncached_write_data(), this line:

	ctx->rc = (rc == 0) ? ctx->total_len : rc;

Hopefully it shows up in your tests.

Cheers,

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Yeah. It finds it. :) It works pretty well. The temptation is to ignore left shifts. Otherwise I think I will just push this.

regards,
dan carpenter

fs/f2fs/segment.c:847 __remove_dirty_segment() warn: check sign expansion for '-1'
fs/cifs/file.c:3177 collect_uncached_write_data() warn: check sign expansion for 'rc' drivers/staging/rtl8188eu/core/rtw_xmit.c:1006 rtw_xmitframe_coalesce() warn: check sign expansion for 'mpdu_len' drivers/usb/gadget/legacy/inode.c:501 ep_aio_complete() warn: check sign expansion for 'req->status' drivers/gpu/drm/nouveau/nouveau_hwmon.c:507 nouveau_in_read() warn: check sign expansion for '-19' drivers/gpu/drm/nouveau/nouveau_hwmon.c:510 nouveau_in_read() warn: check sign expansion for '-19' drivers/firmware/arm_scpi.c:556 scpi_clk_get_val() warn: check sign expansion for 'ret' drivers/clk/sunxi-ng/ccu_nm.c:158 ccu_nm_round_rate() warn: check sign expansion for '1 << nm->m.width' drivers/clk/sunxi-ng/ccu_nm.c:202 ccu_nm_set_rate() warn: check sign expansion for '1 << nm->m.width' drivers/clk/sunxi-ng/ccu_nkmp.c:149 ccu_nkmp_round_rate() warn: check sign expansion for '1 << nkmp->m.width' drivers/clk/sunxi-ng/ccu_nkmp.c:151 ccu_nkmp_round_rate() warn: check sign expansion for '1 << ((1 << nkmp->p.width) - 1)' drivers/clk/sunxi-ng/ccu_nkmp.c:180 ccu_nkmp_set_rate() warn: check sign expansion for '1 << nkmp->m.width' drivers/clk/sunxi-ng/ccu_nkmp.c:182 ccu_nkmp_set_rate() warn: check sign expansion for '1 << ((1 << nkmp->p.width) - 1)' drivers/clk/sunxi-ng/ccu_nkm.c:120 ccu_nkm_round_rate() warn: check sign expansion for '1 << nkm->m.width' drivers/clk/sunxi-ng/ccu_nkm.c:160 ccu_nkm_set_rate() warn: check sign expansion for '1 << nkm->m.width' drivers/net/ethernet/broadcom/bnxt/bnxt.c:9785 bnxt_show_temp() warn: check sign expansion for 'rc' drivers/soc/aspeed/aspeed-lpc-snoop.c:98 snoop_file_read() warn: check sign expansion for 'ret' samples/kfifo/bytestream-example.c:126 fifo_write() warn: check sign expansion for 'ret' samples/kfifo/bytestream-example.c:142 fifo_read() warn: check sign expansion for 'ret' samples/kfifo/record-example.c:133 fifo_write() warn: check sign expansion for 'ret' samples/kfifo/record-example.c:149 fifo_read() warn: check sign expansion for 'ret' samples/kfifo/inttype-example.c:119 fifo_write() warn: check sign expansion for 'ret' samples/kfifo/inttype-example.c:135 fifo_read() warn: check sign expansion for 'ret' net/sunrpc/svcsock.c:1177 svc_tcp_sendto() warn: check sign expansion for 'err'

Great, glad to hear! I didn't check everything but these ones look like real bugs (samples but still):

Cheers,

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Yeah. They mostly are real bugs. I'm going to send fixes for everything tomorrow.

regards,
dan carpenter