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

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

[Aurélien Aptel]

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)
> If both the second and third operands have arithmetic type, the result
> type that would be determined by the usual arithmetic conversions, were
> they applied to those two operands, is the type of the result.

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)

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

[Luc Van Oostenryck]

On Mon, Apr 19, 2021 at 12:21:39PM +0200, Aurélien Aptel wrote:
> 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).

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);

> 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)
> > If both the second and third operands have arithmetic type, the result
> > type that would be determined by the usual arithmetic conversions, were
> > they applied to those two operands, is the type of the result.

Yes, it' also what Sparse is doing.

Cheers,
-- Luc

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

[Dan Carpenter]

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);
}

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

[Aurélien Aptel]

Hi Dan,

Dan Carpenter <dan.carpenter@oracle.com> writes:
> 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.

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

> 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.

Sounds good.

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

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)

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

[Dan Carpenter]

On Tue, Apr 20, 2021 at 02:44:08PM +0200, Aurélien Aptel wrote:
> Hi Dan,
> 
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> > 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.
> 
> I've only used sparse I think (make C=1) I need to lookup how to use Smatch.
> 
> > 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.
> 
> Sounds good.
> 
> > If there are too many false positives when I test this tonight, then I
> > may make is_suspicious_int() more strict.
> 
> 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.        
>

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'

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

[Aurélien Aptel]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> Yeah.  It finds it.  :)  It works pretty well.  The temptation is to
> ignore left shifts.  Otherwise I think I will just push this.

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

> 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'

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)

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

[Dan Carpenter]

On Wed, Apr 21, 2021 at 03:43:03PM +0200, Aurélien Aptel wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> > Yeah.  It finds it.  :)  It works pretty well.  The temptation is to
> > ignore left shifts.  Otherwise I think I will just push this.
> 
> Great, glad to hear! I didn't check everything but these ones look like
> real bugs (samples but still):
> 

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

regards,
dan carpenter