linux-kernel - Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status via OUT arg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1516128562.4042.12.camel@gmail.com>
Date:   Tue, 16 Jan 2018 20:49:22 +0200
From:   Vladislav Valtchev <vladislav.valtchev@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     y.karadz@...il.com, linux-trace-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] trace-cmd: Make read_proc() to return int status
 via OUT arg

On Tue, 2018-01-16 at 11:27 -0500, Steven Rostedt wrote:
> 
> 
> :-)  That was totally lost in translation. :-)
> 
> No, I didn't mean to have a comment literally saying "why would strtol
> return zero and this not be an error", I meant for the comment to
> explain it.
> 
> Actually, looking at the man page which states:
> 

Yep, I got it.
Sometimes I interpret words too literally. My fault :-)

> I say we simply remove the comment. Or say what the man page example
> says:
> 
> 	/* Check for various possible errors */
> 
> and leave it at that.

Sure, "Check for various possible errors" sounds good to me.

> 
> Sure it could be negative. The point was, you don't want it to be if
> you do:
> 
> 	buf[0] = new_status + '0';
> 
> As that will break if new_status is negative or greater than 9.
> 
> Also, whether you use unsigned, or do the above, they both have the
> same result. A negative produces a warning. Which is fine. As long as
> it doesn't kill the program. It's only an implementation detail.
> 
> That is, using unsigned char as new_status, and checking
> 
> 	if (new_status > 9)
> 
> Is no different than using int and checking
> 
> 	if (new_status < 0 || new_status > 9)
> 
> except that you use more instructions to accomplish the same thing.
> 

Sure, using two checks with 'int' is less efficient then using the 'unsigned trick',
but my point is that such a function (at interface level) should accept exactly
the same type 'returned' (via OUT param) by read_proc(). It should be symmetric,
as if instead of 'int/unsigned' we used an opaque type 'value_t' for which we cannot
make assumptions. Clearly, the implementation may in practice accept a subset of the values
allowed by the parameter type. 

What about accepting 'int' but doing the check this way:

	if ((unsigned)new_status > 9) {
		warning(...);
		return;
	}

This way, we'll keep the interface symmetric (with read_proc()) but, at the same time,
we use a more efficient check.

-- 
Vladislav Valtchev
VMware Open Source Technology Center