missing check after calling type_size in classify_x86_64_arg

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

missing check after calling type_size in classify_x86_64_arg

Pascal Cuoq
Hello,

the function type_size can fail and return -1 for an incomplete enum:


In this case it leaves *a untouched.

When this happens when called from the function classify_x86_64_arg, it leads to using the automatic variable align uninitialized:


This scenario happens for some inputs files. I expect all inputs files that cause this to be invalid C programs, but a compiler that emits an error on invalid inputs is better than a compiler that displays undefined behavior on invalid inputs. An example of an input file causing execution to go through classify_x86_64_arg with type_size returning -1 is the following:

enum t f(int x) {
  while(1);
}

I was thinking of inserting a check like “if (size < 0) tcc_error("incomplete enum");” after the call to type_size in classify_x86_64_arg.

The function type_size is called from a lot of places so I didn't even consider making it abort directly instead, but if someone suggests it might be better I can look into it.

Pascal


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Michael Matz-4
Hi,

On Thu, 20 Jun 2019, Pascal Cuoq wrote:

> This scenario happens for some inputs files. I expect all inputs files that
> cause this to be invalid C programs, but a compiler that emits an error on
> invalid inputs is better than a compiler that displays undefined behavior on
> invalid inputs. An example of an input file causing execution to go
> through classify_x86_64_arg with type_size returning -1 is the following:
>
> enum t f(int x) {
>   while(1);
> }
>
> I was thinking of inserting a check like “if (size < 0)
> tcc_error("incomplete enum");” after the call to type_size in
> classify_x86_64_arg.
>
> The function type_size is called from a lot of places so I didn't even
> consider making it abort directly instead, but if someone suggests it
> might be better I can look into it.
Yes, there are generally two contexts, and in one of them (e.g. decls with
initializers) incomplete types are temporarily valid.  So you'd either
need two modes of type_size (one complaining), or two functions (or, as
now, checking the size sometimes).  If you want to invest more work than
just adding a check in classify_x86_64_arg, instead add a function
ctype_size (c for complete) which complains on incomplete types, and use
it in places where the code really needs complete types (and doesn't yet
check on its own).


Ciao,
Michael.
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Pascal Cuoq
Hello Michael, and thanks for the guidance.

On 22 Jun 2019, at 01:17, Michael Matz <[hidden email]> wrote:

Yes, there are generally two contexts, and in one of them (e.g. decls with initializers) incomplete types are temporarily valid.  So you'd either need two modes of type_size (one complaining), or two functions (or, as now, checking the size sometimes).  If you want to invest more work than just adding a check in classify_x86_64_arg, instead add a function ctype_size (c for complete) which complains on incomplete types, and use it in places where the code really needs complete types (and doesn't yet check on its own).

That is a big can of worm you have pointed me to. Here is another part of the code that seems wrong and continues to seem wrong even with the suggested change:

static void struct_layout(CType *type, AttributeDef *ad)
{
    int size, align, maxalign, offset, c, bit_pos, bit_size;
    int packed, a, bt, prevbt, prev_bit_size;
    int pcc = !tcc_state->ms_bitfields;
    int pragma_pack = *tcc_state->pack_stack_ptr;
    Sym *f;

    maxalign = 1;
    offset = 0;
    c = 0;
    bit_pos = 0;
    prevbt = VT_STRUCT; /* make it never match */
    prev_bit_size = 0;

//#define BF_DEBUG

    for (f = type->ref->next; f; f = f->next) {
        if (f->type.t & VT_BITFIELD)
            bit_size = BIT_SIZE(f->type.t);
        else
            bit_size = -1;
        // call type_size here, because t->type can be incomplete
        // if it is a flexible array member
        size = type_size(&f->type, &align);
        a = f->a.aligned ? 1 << (f->a.aligned - 1) : 0;
        packed = 0;

        if (pcc && bit_size == 0) {
            /* in pcc mode, packing does not affect zero-width bitfields */

        } else {
            /* in pcc mode, attribute packed overrides if set. */
            if (pcc && (f->a.packed || ad->a.packed))
                align = packed = 1;

            /* pragma pack overrides align if lesser and packs bitfields always */
            if (pragma_pack) {
                packed = 1;
                if (pragma_pack < align)
                    align = pragma_pack;

align starts its life as an automatic, uninitialized variable. At each iteration, the call to type_size sets it unless the call fails and leaves align's previous value in it.

My only change so far in this function is the comment “call type_size here, because t->type can be incomplete if it is a flexible array member”: I stand by this comment, because calling ctype_size here makes TCC abort while compiling pcctest.c.

Next if pcc is false and pragma_pack is true, the value of align is used in if (pragma_pack < align)

For a flexible array member of a complete element type, this currently works out fine : size_type stores the element type's alignment even if the array is a FAM.

Now consider a FAM of an incomplete enum:

struct s {
  char a;
  enum e b[];
} s;

Fortunately TCC rejects structs that do not have at least one member of a complete type before the FAM, but still, in the above example the value of align that is used in if (pragma_pack < align) is the value from the previous iteration, that is, the alignment from the previous member, and that seems awfully wrong (the program should simply be rejected before reaching that point).

To make a long story short, as the most pressing improvement to clarify the behavior of TCC with incomplete types, I find myself trying to make type_size reject each of following struct declarations:

$ cat t.c
struct s {
  char a;
  enum e b[];
} s;

struct t {
  int a[3];
  void b[];
} t;

typedef void u;

struct v {
  int a[3];
  u b[];
} v;

struct w {
  int a[3];
  struct n b[];
} w;

int printf(const char *, ...);

int main(void) {
  printf("stringlit: %zu\n", sizeof "abcd");
  printf("%zu\n", sizeof s);
  printf("%p\n", &s);
  printf("%p\n", &s.b);
  printf("%zu\n", sizeof(u));
  printf("%zu\n", sizeof t);
  printf("%zu\n", sizeof v);
  printf("%zu\n", sizeof w);
}
pascal@TrustInSoft-Box-VII:~/tinycc_mob$ ./tcc -run t.c
stringlit: 5
1
0xfd6778
0xfd6779
1
12
12
0


And the changes I currently have for this purpose are the following, but they are not sufficient to make TCC reject the declaration of w of type struct w

 ST_FUNC int type_size(CType *type, int *a)
 {
     Sym *s;
@@ -2786,11 +2798,14 @@ ST_FUNC int type_size(CType *type, int *a)
             int ts;

 

             s = type->ref;
+            if ((s->type.t & VT_BTYPE) == VT_VOID)
+                tcc_error("array of void");
             ts = type_size(&s->type, a);
-
-            if (ts < 0 && s->c < 0)
+            if (ts < 0 && s->c < 0) {
+                if (IS_ENUM(s->type.t))
+                    tcc_error("array of incomplete enum");
                 ts = -ts;
-
+            }
             return ts * s->c;
         } else {
             *a = PTR_SIZE;




_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Michael Matz-4
Hello,

On Sat, 22 Jun 2019, Pascal Cuoq wrote:

> That is a big can of worm you have pointed me to.

Historically TCC hasn't cared much about invalid input, so yeah, there be
dragons :)

> Here is another part of the code that seems wrong and continues to seem
> wrong even with the suggested change:

... struct_layout excerpt ...

> align starts its life as an automatic, uninitialized variable. At each
> iteration, the call to type_size sets it unless the call fails and
> leaves align's previous value in it.
>
> My only change so far in this function is the comment “call type_size here,
> because t->type can be incomplete if it is a flexible array member”: I stand
> by this comment, because calling ctype_size here makes TCC abort while
> compiling pcctest.c.

Sure, at this point you can't require a complete type (for the reasons you
state).

> For a flexible array member of a complete element type, this currently works
> out fine : size_type stores the element type's alignment even if the array
> is a FAM.

Yeah, that's the idea.

> struct s {
>   char a;
>   enum e b[];
> } s;
>
> struct t {
>   int a[3];
>   void b[];
> } t;
>
> typedef void u;
>
> struct v {
>   int a[3];
>   u b[];
> } v;
>
> struct w {
>   int a[3];
>   struct n b[];
> } w;
The thing to realize about all these invalid examples is, that it's not
the struct decl which is wrong, i.e. you don't need to change anything
within struct_decl or struct_layout.  It's already the array declarator
itself which is wrong: an array declarator requires a complete element
type.

So, what you want to change is post_type (which cares for array and
function declarators, given a base type) so that the incoming type is
complete if necessary.


Ciao,
Michael.
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Pascal Cuoq

On 22 Jun 2019, at 21:16, Michael Matz <[hidden email]> wrote:

struct s {
  char a;
  enum e b[];
} s;
struct t {
  int a[3];
  void b[];
} t;
typedef void u;
struct v {
  int a[3];
  u b[];
} v;
struct w {
  int a[3];
  struct n b[];
} w;

The thing to realize about all these invalid examples is, that it's not the struct decl which is wrong, i.e. you don't need to change anything within struct_decl or struct_layout.  It's already the array declarator itself which is wrong: an array declarator requires a complete element type.

So, what you want to change is post_type (which cares for array and function declarators, given a base type) so that the incoming type is complete if necessary.

So, pushing the previous stuff down the stack and investigating post_type, it is currently making a minimal effort to reject arrays of functions, and I tried to insert a better filter at the place where the check was (see attached patch). The new version continues to reject this example:

typedef void f(void);

f t[3];

(with a more generic message)
The new version also rejects all my previous examples quoted above, and the following, which is currently accepted because type->t is not masked in the current implementation:

const f t[3];

If this patch can be tweaked into something acceptable, I will also add tests for the new rejected constructs and validate the message change for the existing test.

Pascal

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

arrays_of_incomplete.patch (1K) Download Attachment
ATT00001.htm (322 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Michael Matz-4
Hi,

On Sun, 23 Jun 2019, Pascal Cuoq wrote:

> If this patch can be tweaked into something acceptable, I will also add
> tests for the new rejected constructs and validate the message change
> for the existing test.

The patch definitely goes into the right direction, though it seem more
verbose than necessary.  I'd just test for functions or incomplete types
(via type_size), and then you have the opportunity to retain the more
precise error message for the former, ala

   if (func)
     tcc_error ...
   else if (type_size < 0)
     tcc_error ...
   okay ...


Ciao,
Michael.

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Pascal Cuoq
Hello,

> On 23 Jun 2019, at 23:18, Michael Matz <[hidden email]> wrote:
>
> The patch definitely goes into the right direction, though it seem more verbose than necessary.  I'd just test for functions or incomplete types (via type_size), and then you have the opportunity to retain the more precise error message for the former, ala
>
>  if (func)
>    tcc_error ...
>  else if (type_size < 0)
>    tcc_error ...
>  okay ...

I considered this but:
- the single generic error message seemed consistent with a compiler which advertises its small size and its speed, and which was accepting these programs until recently,
- but even producing a generic error message, functions would have to remain as a separate case because type_size returns 1 for them,
- and also type_size returns 1 for void, so that would have to be another separate case if we wish to reject arrays of void.

Two separate cases mean we have to compute type->t & VT_BTYPE. If we call type_size it will recompute it, and store an alignment that we do not require to a variable that we will have to declare. It doesn't look like it's shorter or faster.

If you think that this much code should be put in a function, that might be useful elsewhere, I can do that, but when compiling GCC's dialect of C where pointer arithmetic is legal for pointers to void and function pointers, “being a type with a size in the sense of pointer arithmetic” and “being a complete type” become different enough to require two different functions.
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Michael Matz-4
Hi,

On Sun, 23 Jun 2019, Pascal Cuoq wrote:

>> The patch definitely goes into the right direction, though it seem more verbose than necessary.  I'd just test for functions or incomplete types (via type_size), and then you have the opportunity to retain the more precise error message for the former, ala
>>
>>  if (func)
>>    tcc_error ...
>>  else if (type_size < 0)
>>    tcc_error ...
>>  okay ...
>
> I considered this but:
> - the single generic error message seemed consistent with a compiler which advertises its small size and its speed, and which was accepting these programs until recently,
> - but even producing a generic error message, functions would have to remain as a separate case because type_size returns 1 for them,
> - and also type_size returns 1 for void, so that would have to be another separate case if we wish to reject arrays of void.

Sure, but to avoid two cases you added five, and encoded the knowledge
about the ->c field in still another place.  What I meant is adding two
lines like so:

diff --git a/tccgen.c b/tccgen.c
index c65cf76..84e0953 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -4507,6 +4507,8 @@ static int post_type(CType *type, AttributeDef *ad, int storage, int td)
          post_type(type, ad, storage, 0);
          if (type->t == VT_FUNC)
              tcc_error("declaration of an array of functions");
+        else if ((type->t & VT_BTYPE) == VT_VOID || type_size(type, &l) < 0)
+            tcc_error("wrong!");
          t1 |= type->t & VT_VLA;

          if (t1 & VT_VLA) {

> Two separate cases mean we have to compute type->t & VT_BTYPE. If we
> call type_size it will recompute it, and store an alignment that we do
> not require to a variable that we will have to declare. It doesn't look
> like it's shorter or faster.

It's definitely shorter, see above :)  If you consider
compilation speed you'd have to compare with an optimizing compiler
anyway, and then none of the above unoptimality matters.  Even without an
optimizing compiler (say, with TCC itself) the code generated for the
switch will be larger than the above.

Thing is: we have the type_size helper routine, so it should be used.

In the end of course some of this is merely a question about style and
that's subjective.  Personally I like things being done in fewer lines of
code (without breaking the formating style) first, and then
optimize hot spots.  This place definitely isn't one.

> If you think that this much code should be put in a function, that might
> be useful elsewhere, I can do that,

Only if multiple users are added; the block (even your version) isn't
complex enough to warrant a separate function with just a single caller
IMHO.


Ciao,
Michael.

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Pascal Cuoq

> On 24 Jun 2019, at 05:01, Michael Matz <[hidden email]> wrote:
>
> diff --git a/tccgen.c b/tccgen.c
> index c65cf76..84e0953 100644
> --- a/tccgen.c
> +++ b/tccgen.c
> @@ -4507,6 +4507,8 @@ static int post_type(CType *type, AttributeDef *ad, int storage, int td)
>         post_type(type, ad, storage, 0);
>         if (type->t == VT_FUNC)
>             tcc_error("declaration of an array of functions");
> +        else if ((type->t & VT_BTYPE) == VT_VOID || type_size(type, &l) < 0)
> +            tcc_error("wrong!");
>         t1 |= type->t & VT_VLA;
>
>         if (t1 & VT_VLA) {

As you wish, but I still intend to fix the “arrays of const functions” bug at the same time. Also I declared a different variable align_unused because, if I used the existing variable align, I wouldn't want the next person reading this function, who might be me, to wonder whether the value from this call is used on purpose later.

Pushed to mob.

Pascal

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

grischka
In reply to this post by Pascal Cuoq
Pascal Cuoq wrote:

       int t[][3];  [ error: unknown type size ]

Actually that is not wrong.  It's the same as

       extern int t[][3];

MSVC accepts it without warning, and GCC too if either 'extern' or
the size is still specified:

       int t[][3];
       extern int t[][3];
or
       int t[][3] = {{1,2,3}};

I'v pushed a patch that does allow that (among other things ;)
https://repo.or.cz/tinycc.git/commitdiff/85690480313c4a8b0efeb3761ee68456cfe57840

W.r.t. changes on VT_EXTERN, they were originally based on the idea
that I wanted the distinction between definition and declaration to
be provided on the Sym level (rather than the ESym one).

-- gr


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: missing check after calling type_size in classify_x86_64_arg

Pascal Cuoq
Hello,

> On 24 Jun 2019, at 12:05, grischka <[hidden email]> wrote:
>
> Pascal Cuoq wrote:
>
>      int t[][3];  [ error: unknown type size ]
>
> Actually that is not wrong.  It's the same as
>
>      extern int t[][3];

Ah yes. Anyway my patch did not change the behavior for this example. I wrote the test intending “int t[3][];”, I think, and got it wrong.
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel