www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - About some bugs

reply bearophile <bearophileHUGS lycos.com> writes:
I have studied more Linux bugs.

----------------

An example of bug (more than 14 like this fixed in few years):

-       memset(pp, 0, sizeof(pp));
+       memset(pp, 0, sizeof(*pp));

-       memcpy((caddr_t)TstSchedTbl, (caddr_t)&vcIndex,sizeof(TstSchedTbl));
+       memcpy((caddr_t)TstSchedTbl, (caddr_t)&vcIndex, sizeof(*TstSchedTbl));

Here the type system knows that pp is a pointer. sizeof(pp) is typically a
word, while the correct sizeof(*pp) is often larger. A simple way to avoid this
bug in D is to use a zerioing template function, something like (untested) (in
GNU C there is a way to write a similar macro, I don't know why they don't use
it, even if it's a bit less safe and much less nice looking):

void zeroit(T)(T* ptr) if (!IsPointer!T) {
    memset(ptr, 0, (*ptr).sizeof);
}

Standard safer wrappers for some C functions may help low-level D coding.

If you don't want to use a zeroit() then a type system able to catch such bugs
needs some nice annotations...

----------------

A common (> 12 cases in few years) example of redundant code:

        u32 da = new->da_start;
-       if (!obj || !new || !sgt) {...
+       if (!obj || !sgt) {...

If the first line is correct, then "new" can't be NULL, so there's no need to
test "|| !new".

In 13 similar cases the code is inside a branch where the NULL test of the
pointer is already done:

if (ptr != NULL) {
    if (!ptr) { ...
}

----------------

In 7 cases the result of malloc-like function was not tested for NULL:

        agp = kmalloc(sizeof(*agp), GFP_KERNEL);
+       if (!agp)
+               return NULL;

----------------

A very common case (20 cases in few years) are like this, where a pointer is
deferenced before the NULL test:

        block = bdev->bd_disk->private_data;
-       base = block->base;
        if (!block)
                return -ENODEV;
+       base = block->base;

----------------

2 cases like this (the pattern (!E && !E->fld) is nonsensical):

-               if (!slot->hotplug_slot && !slot->hotplug_slot->info) 
+               if (!slot->hotplug_slot || !slot->hotplug_slot->info)
                        continue;

----------------

There are 11 cases like this (alloc_bootmem never returns NULL and it always
sets the momory to zero):

    zero_page = alloc_bootmem_low_pages_node(NODE_DATA(0), PAGE_SIZE);
-   memset(zero_page, 0, PAGE_SIZE);

----------------

There are a lot of cases (34) of missing free:

+       kfree(iter);

----------------

Two cases need a wider NULL test:

-               if (fep != NULL) {
+               if (fep && fep->ops) {
                        (*fep->ops->free_bd)(ndev);
                        (*fep->ops->cleanup_data)(ndev);
                }

----------------

In this post I don't see any little rule worth adding to the D compiler. So to
bugzilla I will add only the !x&y thing.

But from what I have seen many bugs are NULL-related, and many of them are
avoidable with two features present at the same time in the language:

1) Nonnull pointer/reference tag (with it you don't miss some null tests, and
you avoid some redundant tests);

2) Plus a bit of inter-function (not intra, for that nonnull signatures are
enough) flow analysis to avoid bugs like:

base = block->base;
if (!block)
    return -ENODEV;

if (fep != NULL) {
    (*fep->ops->free_bd)(ndev);

(P.S.: integer overflow bugs happen in Linux kernel too.)

Bye,
bearophile
Jan 04 2011
next sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Tue, 04 Jan 2011 14:34:15 +0200, bearophile <bearophileHUGS lycos.com>  
wrote:

 void zeroit(T)(T* ptr) if (!IsPointer!T) {
     memset(ptr, 0, (*ptr).sizeof);
 }

 Standard safer wrappers for some C functions may help low-level D coding.

 If you don't want to use a zeroit() then a type system able to catch  
 such bugs needs some nice annotations...

Doesn't D already solve this? For value types: obj = typeof(obj).init; For arrays: arr[] = typeof(arr[0]).init; // or just 0 or null or whatever .init is
 If the first line is correct, then "new" can't be NULL, so there's no  
 need to test "|| !new".

I think this is something that should be done by the optimizer.
 In 7 cases the result of malloc-like function was not tested for NULL:

This is rather specific. Application programmers would usually want an exception to be thrown on a failed allocation.
 A very common case (20 cases in few years) are like this, where a  
 pointer is deferenced before the NULL test:

         block = bdev->bd_disk->private_data;
 -       base = block->base;
         if (!block)
                 return -ENODEV;
 +       base = block->base;

Delphi compilers warn in cases when a condition is always true/false, because Delphi lacks metaprogramming so such cases are usually due to a bug. -- Best regards, Vladimir mailto:vladimir thecybershadow.net
Jan 04 2011
parent bearophile <bearophileHUGS lycos.com> writes:
Vladimir Panteleev:

 Doesn't D already solve this?

Yup.
         block = bdev->bd_disk->private_data;
 -       base = block->base;
         if (!block)
                 return -ENODEV;
 +       base = block->base;

Delphi compilers warn in cases when a condition is always true/false, because Delphi lacks metaprogramming so such cases are usually due to a bug.

This is a different situation. You need a bit of flow analisys to catch this bug (to give a state to a type). Bye, bearophile
Jan 04 2011
prev sibling next sibling parent reply spir <denis.spir gmail.com> writes:
On Tue, 04 Jan 2011 07:34:15 -0500
bearophile <bearophileHUGS lycos.com> wrote:

 An example of bug (more than 14 like this fixed in few years):
=20
 -       memset(pp, 0, sizeof(pp));
 +       memset(pp, 0, sizeof(*pp));
=20
 -       memcpy((caddr_t)TstSchedTbl, (caddr_t)&vcIndex,sizeof(TstSchedTbl=

 +       memcpy((caddr_t)TstSchedTbl, (caddr_t)&vcIndex, sizeof(*TstSchedT=

=20
 Here the type system knows that pp is a pointer. sizeof(pp) is typically =

d this bug in D is to use a zerioing template function, something like (unt= ested) (in GNU C there is a way to write a similar macro, I don't know why = they don't use it, even if it's a bit less safe and much less nice looking):
=20
 void zeroit(T)(T* ptr) if (!IsPointer!T) {
     memset(ptr, 0, (*ptr).sizeof);
 }

Doesn't this in fact hide the error to the programmer (by silently correcti= ng)? Why not instead for instance: void zeroit(T)(T* ptr) if (!IsPointer!T) { throw new Exception("Type error: argument to <funcname> should be a poi= nter."); } (And what if the memory to be actually memset is not ptr's target?) About non-null thingies, I would be all for a mode in which is inserted if (p is null) throw ...; before _every_ implicite or explicite deref of every implicite (pointer) or= implicite (class element) pointer. And even make this the default for non-= release. (With line number in the message ;-) Am I dreaming? Denis -- -- -- -- -- -- -- vit esse estrany =E2=98=A3 spir.wikidot.com
Jan 04 2011
parent bearophile <bearophileHUGS lycos.com> writes:
Simen kjaeraas:

 Much more likely would be the idea that has been discussed, of opt-in
 non-nullness, by disabling the default constructor for a struct that
 wraps a pointer.

I am not sure that's enough. Bye, bearophile
Jan 04 2011
prev sibling next sibling parent "Simen kjaeraas" <simen.kjaras gmail.com> writes:
spir <denis.spir gmail.com> wrote:

 void zeroit(T)(T* ptr) if (!IsPointer!T) {
     memset(ptr, 0, (*ptr).sizeof);
 }

Doesn't this in fact hide the error to the programmer (by silently correcting)?

No. It ensures that the pointer and size are from the same pointer. I would say it is somewhat analogous to D's arrays, which are a hell of a lot better than free pointer/length pairs.
 Why not instead for instance:

 void zeroit(T)(T* ptr) if (!IsPointer!T) {
     throw new Exception("Type error: argument to <funcname> should be a  
 pointer.");
 }

I'm not sure this makes sense. The error message seems to indicate you don't have a pointer, while that is exactly what you do. The template constraint also say that this is not an overload of the above function, making it simply a function that throws.
 (And what if the memory to be actually memset is not ptr's target?)

If people start throwing random pointers into functions, that is a problem that's hard to work around. bearophile's zeroit function takes memset and removes its main weak point, namely that it takes a free pointer/length pair, rather than a structure containing the two.
 About non-null thingies, I would be all for a mode in which is inserted
 	if (p is null) throw ...;
 before _every_ implicite or explicite deref of every implicite (pointer)  
 or implicite (class element) pointer. And even make this the default for  
 non-release. (With line number in the message ;-)
 Am I dreaming?

Yeah. :P Much more likely would be the idea that has been discussed, of opt-in non-nullness, by disabling the default constructor for a struct that wraps a pointer. -- Simen
Jan 04 2011
prev sibling next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
bearophile wrote:
 I have studied more Linux bugs.
 
 ----------------
 
 An example of bug (more than 14 like this fixed in few years):
 
 -       memset(pp, 0, sizeof(pp)); +       memset(pp, 0, sizeof(*pp));
 
 -       memcpy((caddr_t)TstSchedTbl, (caddr_t)&vcIndex,sizeof(TstSchedTbl)); 
 +       memcpy((caddr_t)TstSchedTbl, (caddr_t)&vcIndex,
 sizeof(*TstSchedTbl));
 
 Here the type system knows that pp is a pointer. sizeof(pp) is typically a
 word, while the correct sizeof(*pp) is often larger. A simple way to avoid
 this bug in D is to use a zerioing template function, something like
 (untested) (in GNU C there is a way to write a similar macro, I don't know
 why they don't use it, even if it's a bit less safe and much less nice
 looking):
 
 void zeroit(T)(T* ptr) if (!IsPointer!T) { memset(ptr, 0, (*ptr).sizeof); }
 
 Standard safer wrappers for some C functions may help low-level D coding.
 
 If you don't want to use a zeroit() then a type system able to catch such
 bugs needs some nice annotations...

In D: pp[] = 0; or: pp = typeof(pp).init; etc.
 In this post I don't see any little rule worth adding to the D compiler.

Many of them are dealt with with D's scope guard, RIAA, and garbage collection support.
Jan 04 2011
prev sibling next sibling parent "Simen kjaeraas" <simen.kjaras gmail.com> writes:
bearophile <bearophileHUGS lycos.com> wrote:

 Simen kjaeraas:

 Much more likely would be the idea that has been discussed, of opt-in
 non-nullness, by disabling the default constructor for a struct that
 wraps a pointer.

I am not sure that's enough.

Could you please show a case where it isn't? -- Simen
Jan 04 2011
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
bearophile wrote:
 I have studied more Linux bugs.

Is there a research paper that these came from?
Jan 04 2011
parent bearophile <bearophileHUGS lycos.com> writes:
Walter:

 bearophile wrote:
 I have studied more Linux bugs.

Is there a research paper that these came from?

They are just part of the Linux bugs fixed in those few years. I am sorry, but what I have written doesn't come from a paper. They partially come from this page: http://coccinelle.lip6.fr/impact_linux.php Plus some exploration of the kernel bug repository. With some more weeks of search and work a paper may be written :o) Bye, bearophile
Jan 04 2011