lkml.org 
[lkml]   [2015]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.
On Tue, 08 Sep 2015 11:19:14 -0500
ebiederm@xmission.com (Eric W. Biederman) wrote:


> This patch does not implement the old behavior.
>
> The old code does use '\0' as a buffer terminator, and because it does
> not check things closely I can see how it could accept a '\0' from
> userspace and treat that as an early buffer terminator.
>
> The patch treats '\0' as a number separator and allows things that have
> never been allowed before and quite frankly is very scary as it just
> invites bugs.
>
> So I do not think we should merge the given patch. It is just wrong.
> One that simply truncates the input buffer at the first '\0' character I
> think we can consider, although I am not a fan.

I agree, and was thinking this patch did that, but I didn't look close
enough (why I never gave a Reviewed-by to it either).


>
> Steve as far as what I think would break. I don't think the current
> behavior should have broken anything and apparently it did. I don't see
> what a change that simply truncates the buffer at the first embedded
> '\0' would break, but I don't know how to test that there isn't anything
> that it will. We are way past the point of reasonable expectations
> being able to guide us. 4 years should have been more than enough soak
> time to have been able to say that the change was good, but apparently
> it was not.

Well, to be fair, a lot of people (distros, etc) do not use the most
recent kernels. 4 years may be the first time a tool touches a kernel.

>
> My gut feel says that if we are going to change this, at this late date,
> we find the one specific proc file that matters and change it just for
> that one proc file, and in that change we treat '\0' as a terminator not
> as a separator. I never did see in the conversation which proc file it
> is that actually matters. The principle is that the more precise and
> the more localized such a change is the less chance it has of causing a
> regression of something else, and the greater the chance we can look at
> a specific issue.

Sounds like a reasonable compromise. Sean, can you make a patch that
only affects the one proc file (comment it well in the code), and have
it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
would only see "1 "

-- Steve


\
 
 \ /
  Last update: 2015-09-08 19:01    [W:0.096 / U:2.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site