Re: [Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Iago Toral]

Hi Juha,

On Sun, 2016-02-07 at 23:37 +0200, Juha-Pekka Heikkilä wrote:
> Hi Iago,
> 
> I know there are lot of places where there is malloc unchecked still
> -- and then there is ralloc which is a story of its own. Reason why I
> think checking these would be remotely useful in windows only (or
> other way around, not under linux kernel) is on Windows one can get
> the null pointer from malloc. On Androids I think memory over
> committing has always been enabled and on Linux I suspect I belong to
> the minority who like to set ulimits for memory.
> 
> I agree checking these mostly is quite useless but there are those
> corners where it may suddenly become valuable. When process is running
> and everything has settled it will be weird if hit any of these checks
> but any code which is run when process is starting I notice is the
> place where things will fail if they fail. This is of course just my
> opinion about the value of these checks but I really dislike
> possibility of segfault when it is coming from a library.
> 
> I didn't quickly notice where _mesa_error() get more heap. Stack it of
> course needs but when I did stress test these _mesa_error() did still
> work. Cannot promise my test was 100% correct though, I think it was
> over year ago when I was playing with it.
> 

I think Matt explained this but I suppose it might still help in some
platforms. Ian's point about using NULL as an offset is also valid.

Anyway, I am not blocking anything, the patches are a small thing and
they might help in some cases so feel to go ahead with them. If you
still need a Reviewed-by for these you can add mine or Samuel's.

Iago

> 
> On Wed, Feb 3, 2016 at 5:12 PM, Iago Toral  wrote:
> > Hi Juha,
> >
> > I don't know why checking for this might be more relevant in Windows,
> > but in any case:
> >
> > There are a ton of other places in mesa where we allocate memory via
> > calloc/malloc and we don't check that the allocation actually succeeded
> > so I am not sure that fixing a couple of instances of *small*
> > allocations changes anything.
> >
> > IMHO, this kind of things are only really useful when allocating memory
> > for large amounts of data, otherwise even if you check for a NULL
> > allocation you still need to make sure that you don't need any extra
> > memory to handle that situation, and _mesa_error() needs memory, so it
> > is probably not really giving us anything in practice other than
> > silencing Klocwork...
> >
> > Iago
> >
> > On Wed, 2016-02-03 at 10:56 +0200, Juha-Pekka Heikkila wrote:
> >> I'm thinking these things maybe could be wrapped up inside something like
> >> "#ifdef windows" or so in the future. At least for Android and Linux these
> >> are normally quite useless.
> >>
> >> /Juha-Pekka
> >>
> >> Juha-Pekka Heikkila (2):
> >>   i965: in brw_link_shader() react to low memory
> >>   glsl: Check for null pointer at ir_variable_refcount_visitor()
> >>
> >>  src/compiler/glsl/ir_variable_refcount.cpp | 7 +++
> >>  src/mesa/drivers/dri/i965/brw_link.cpp | 4 
> >>  src/mesa/main/ff_fragment_shader.cpp   | 6 --
> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >
> >
> 


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Eero Tamminen]

Hi,

On 08.02.2016 00:07, Matt Turner wrote:

On Sun, Feb 7, 2016 at 1:37 PM, Juha-Pekka Heikkilä
 wrote:

I know there are lot of places where there is malloc unchecked still
-- and then there is ralloc which is a story of its own. Reason why I
think checking these would be remotely useful in windows only (or
other way around, not under linux kernel) is on Windows one can get
the null pointer from malloc. On Androids I think memory over
committing has always been enabled and on Linux I suspect I belong to
the minority who like to set ulimits for memory.

I agree checking these mostly is quite useless but there are those
corners where it may suddenly become valuable. When process is running
and everything has settled it will be weird if hit any of these checks
but any code which is run when process is starting I notice is the
place where things will fail if they fail. This is of course just my
opinion about the value of these checks but I really dislike
possibility of segfault when it is coming from a library.

I didn't quickly notice where _mesa_error() get more heap. Stack it of
course needs but when I did stress test these _mesa_error() did still
work. Cannot promise my test was 100% correct though, I think it was
over year ago when I was playing with it.


There's no guarantee that fprintf() doesn't call malloc. In fact, glibc's does.


If one is just concerned about not calling something that may use 
malloc, the functions listed by "man 7 signal" as async-signal-safe are 
safe in that respect (one cannot use malloc within signal handler context).




Adding these checks is really useless.


If one runs out of memory when overcommit is enabled, in theory about 
anything the program does, can cause it to use more memory.  Not just 
allocations.   E.g. running code for handling an error, may cause kernel 
to need to allocate space to page that code into RAM.  This can happen 
even if the isn't run for the first time, if kernel had dropped that 
page from RAM.



- Eero

(Modern desktop systems don't work well without overcommit.  Having 
system with programs that have large number of threads or otherwise 
hardly used memory mapping, like is case e.g. on fork, could be in 
trouble, unless they have overtly large amounts of RAM.)


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Ian Romanick]

On 02/07/2016 02:07 PM, Matt Turner wrote:
> On Sun, Feb 7, 2016 at 1:37 PM, Juha-Pekka Heikkilä
>  wrote:
>> Hi Iago,
>>
>> I know there are lot of places where there is malloc unchecked still
>> -- and then there is ralloc which is a story of its own. Reason why I
>> think checking these would be remotely useful in windows only (or
>> other way around, not under linux kernel) is on Windows one can get
>> the null pointer from malloc. On Androids I think memory over
>> committing has always been enabled and on Linux I suspect I belong to
>> the minority who like to set ulimits for memory.
>>
>> I agree checking these mostly is quite useless but there are those
>> corners where it may suddenly become valuable. When process is running
>> and everything has settled it will be weird if hit any of these checks
>> but any code which is run when process is starting I notice is the
>> place where things will fail if they fail. This is of course just my
>> opinion about the value of these checks but I really dislike
>> possibility of segfault when it is coming from a library.
>>
>> I didn't quickly notice where _mesa_error() get more heap. Stack it of
>> course needs but when I did stress test these _mesa_error() did still
>> work. Cannot promise my test was 100% correct though, I think it was
>> over year ago when I was playing with it.
> 
> There's no guarantee that fprintf() doesn't call malloc. In fact, glibc's 
> does.
> 
> Adding these checks is really useless.

Maybe.  What Klocwork tries to detect isn't that your program might
crash because of a failed allocation.  It's trying to detect that malloc
returns NULL, then, perhaps through a series of offset operations, you
reference some other valid memory and get a weird result that could be
used for an exploit.  It's super paranoid and overly conservative in
these estimations.

This

*(char *)NULL = 'x';

will crash.  This

((int *)NULL)[some_value] = 42;

is much less predictable... especially if some_value comes directly or
indirectly from user input.  Since the dereference may occur far, far
away from the allocation, it's halting-problem hard for a static
analysis tool to determine it cannot happen.

That said, I think the way we're currently adding this checks is pretty
much rubbish.  There's no testing, and, as a result, an unfortunate
percentage of the added tests have had bugs or one sort or another.

> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Matt Turner]

On Sun, Feb 7, 2016 at 1:37 PM, Juha-Pekka Heikkilä
 wrote:
> Hi Iago,
>
> I know there are lot of places where there is malloc unchecked still
> -- and then there is ralloc which is a story of its own. Reason why I
> think checking these would be remotely useful in windows only (or
> other way around, not under linux kernel) is on Windows one can get
> the null pointer from malloc. On Androids I think memory over
> committing has always been enabled and on Linux I suspect I belong to
> the minority who like to set ulimits for memory.
>
> I agree checking these mostly is quite useless but there are those
> corners where it may suddenly become valuable. When process is running
> and everything has settled it will be weird if hit any of these checks
> but any code which is run when process is starting I notice is the
> place where things will fail if they fail. This is of course just my
> opinion about the value of these checks but I really dislike
> possibility of segfault when it is coming from a library.
>
> I didn't quickly notice where _mesa_error() get more heap. Stack it of
> course needs but when I did stress test these _mesa_error() did still
> work. Cannot promise my test was 100% correct though, I think it was
> over year ago when I was playing with it.

There's no guarantee that fprintf() doesn't call malloc. In fact, glibc's does.

Adding these checks is really useless.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Juha-Pekka Heikkilä]

Hi Iago,

I know there are lot of places where there is malloc unchecked still
-- and then there is ralloc which is a story of its own. Reason why I
think checking these would be remotely useful in windows only (or
other way around, not under linux kernel) is on Windows one can get
the null pointer from malloc. On Androids I think memory over
committing has always been enabled and on Linux I suspect I belong to
the minority who like to set ulimits for memory.

I agree checking these mostly is quite useless but there are those
corners where it may suddenly become valuable. When process is running
and everything has settled it will be weird if hit any of these checks
but any code which is run when process is starting I notice is the
place where things will fail if they fail. This is of course just my
opinion about the value of these checks but I really dislike
possibility of segfault when it is coming from a library.

I didn't quickly notice where _mesa_error() get more heap. Stack it of
course needs but when I did stress test these _mesa_error() did still
work. Cannot promise my test was 100% correct though, I think it was
over year ago when I was playing with it.

/Juha-Pekka

On Wed, Feb 3, 2016 at 5:12 PM, Iago Toral  wrote:
> Hi Juha,
>
> I don't know why checking for this might be more relevant in Windows,
> but in any case:
>
> There are a ton of other places in mesa where we allocate memory via
> calloc/malloc and we don't check that the allocation actually succeeded
> so I am not sure that fixing a couple of instances of *small*
> allocations changes anything.
>
> IMHO, this kind of things are only really useful when allocating memory
> for large amounts of data, otherwise even if you check for a NULL
> allocation you still need to make sure that you don't need any extra
> memory to handle that situation, and _mesa_error() needs memory, so it
> is probably not really giving us anything in practice other than
> silencing Klocwork...
>
> Iago
>
> On Wed, 2016-02-03 at 10:56 +0200, Juha-Pekka Heikkila wrote:
>> I'm thinking these things maybe could be wrapped up inside something like
>> "#ifdef windows" or so in the future. At least for Android and Linux these
>> are normally quite useless.
>>
>> /Juha-Pekka
>>
>> Juha-Pekka Heikkila (2):
>>   i965: in brw_link_shader() react to low memory
>>   glsl: Check for null pointer at ir_variable_refcount_visitor()
>>
>>  src/compiler/glsl/ir_variable_refcount.cpp | 7 +++
>>  src/mesa/drivers/dri/i965/brw_link.cpp | 4 
>>  src/mesa/main/ff_fragment_shader.cpp   | 6 --
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Juha-Pekka Heikkila]

I'm thinking these things maybe could be wrapped up inside something like
"#ifdef windows" or so in the future. At least for Android and Linux these
are normally quite useless.

/Juha-Pekka

Juha-Pekka Heikkila (2):
  i965: in brw_link_shader() react to low memory
  glsl: Check for null pointer at ir_variable_refcount_visitor()

 src/compiler/glsl/ir_variable_refcount.cpp | 7 +++
 src/mesa/drivers/dri/i965/brw_link.cpp | 4 
 src/mesa/main/ff_fragment_shader.cpp   | 6 --
 3 files changed, 15 insertions(+), 2 deletions(-)

-- 
1.9.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Iago Toral]

Hi Juha,

I don't know why checking for this might be more relevant in Windows,
but in any case:

There are a ton of other places in mesa where we allocate memory via
calloc/malloc and we don't check that the allocation actually succeeded
so I am not sure that fixing a couple of instances of *small*
allocations changes anything.

IMHO, this kind of things are only really useful when allocating memory
for large amounts of data, otherwise even if you check for a NULL
allocation you still need to make sure that you don't need any extra
memory to handle that situation, and _mesa_error() needs memory, so it
is probably not really giving us anything in practice other than
silencing Klocwork...

Iago

On Wed, 2016-02-03 at 10:56 +0200, Juha-Pekka Heikkila wrote:
> I'm thinking these things maybe could be wrapped up inside something like
> "#ifdef windows" or so in the future. At least for Android and Linux these
> are normally quite useless.
> 
> /Juha-Pekka
> 
> Juha-Pekka Heikkila (2):
>   i965: in brw_link_shader() react to low memory
>   glsl: Check for null pointer at ir_variable_refcount_visitor()
> 
>  src/compiler/glsl/ir_variable_refcount.cpp | 7 +++
>  src/mesa/drivers/dri/i965/brw_link.cpp | 4 
>  src/mesa/main/ff_fragment_shader.cpp   | 6 --
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev