gcc / clib bug?

[bnemias]

I don't need to know the specific reason it crashed. It's poor coding practice. Why not just have name be a pointer, and allocate it separately as has been done since forever for this type of thing? There's even a string duplication function in that project iirc.

Separately allocating the string is less efficient and the reason to code in C is often to not have to put up with such inefficiencies.

The inefficiency is negligible. Compare that to the manpower devoted to observing the crash, notifying the maintainer, tracking the bug on a different OS than the reporter, and fixing it.

Write boring and sometimes less than efficient code. Save tons of time.

If you ran the less efficient routine 1 million times, you still wouldn't detect it without using software designed to detect it.

[syzygy]

I don't need to know the specific reason it crashed. It's poor coding practice. Why not just have name be a pointer, and allocate it separately as has been done since forever for this type of thing? There's even a string duplication function in that project iirc.

Separately allocating the string is less efficient and the reason to code in C is often to not have to put up with such inefficiencies.

The inefficiency is negligible. Compare that to the manpower devoted to observing the crash, notifying the maintainer, tracking the bug on a different OS than the reporter, and fixing it.

What bug? The code is not buggy. (I now believe it does not even rely on compiler behaviour not mandated by the standard. The result of &(*i)->name is a pointer to a char and there is guaranteed to be sufficient space. The issue raised by Álvaro has to do with indexing a 1-element array beyond its bound.)

Imho the code would be nicer if it used a flexible array member (char name[]) (and if it did not use "i" for something that is not an int ).

I assume you consider the use of a flexible array member to be "bad coding practice". I wonder why that is. It is simpler than the solution you propose. It is also more efficient.

Efficiency is often not a big deal, and I'm sure it is not a big deal for this particular piece of code, but I don't see a good reason for choosing a less efficient solution that is more work and allows for more mistakes.

And again, this is C. In C everything is dangerous.

[bnemias]

What bug? The code is not buggy.

name is defined as a char, but it's not a char. in fact, why even have that member when you could just index the memory directly with casts and pointer addition? name is merely a placeholder for something else.

If you consider using a variable in ways it wasn't defined not to be a bug, be my guest.

And again, this is C. In C everything is dangerous.

C isn't inherently dangerous. Using risky code is. Just because it doesn't explicitly prohibit bad behavior doesn't mean it can't be used safely. Writing solid code is a mindset irrespective of language.

[syzygy]

What bug? The code is not buggy.

name is defined as a char, but it's not a char. in fact, why even have that member when you could just index the memory directly with casts and pointer addition? name is merely a placeholder for something else.

If you consider using a variable in ways it wasn't defined not to be a bug, be my guest.

I thought you were talking about a bug that needs to be tracked and fixed.

Anyway, I don't consider code that always does exactly what it was intended to do buggy. And the char ends up storing a char. Add [1] behind it and you have a code construct that has been used since forever (but is not completely supported by the standard, a problem that hgm's code does not have).

But how about this?

Code: Select all

 struct t_tree {
    struct t_tree *left, *right;
    char name[];
  };
  
  struct t_dirs {
    struct t_dirs *left, *right;
    time_t mtime;                 /* dir's modification time */
    struct t_tree *files;
    char name[];
  };

// in searc_directory():

      if (!*p) {                          /* if dir isn't in dir tree, add him */
        *p = malloc(sizeof(struct t_dirs) + strlen(dir) + 1);
        (*p)->left = (*p)->right = NULL;
        (*p)->files = NULL;
        strcpy((*p)->name, dir);
      }

Buggy? Poor coding practice? Or fine?

And again, this is C. In C everything is dangerous.

C isn't inherently dangerous. Using risky code is. Just because it doesn't explicitly prohibit bad behavior doesn't mean it can't be used safely. Writing solid code is a mindset irrespective of language.

How is the above code more risky than:

Code: Select all

 struct t_tree {
    struct t_tree *left, *right;
    char *name;
  };
  
  struct t_dirs {
    struct t_dirs *left, *right;
    time_t mtime;                 /* dir's modification time */
    struct t_tree *files;
    char *name;
  };

// in searc_directory():

      if (!*p) {                          /* if dir isn't in dir tree, add him */
        *p = malloc(sizeof(struct t_dirs));
        (*p)->left = (*p)->right = NULL;
        (*p)->files = NULL;
        (*p)->name = malloc(strlen(dir) + 1);
        strcpy((*p)->name, dir);
      }

Now you have to remember free()ing (*p)->name, so I only see more "risk".

I have nothing against solid code. My point is the code was already solid.

[bnemias]

But how about this?

Code: Select all

 struct t_tree {
    struct t_tree *left, *right;
    char name[];
  };
  
  struct t_dirs {
    struct t_dirs *left, *right;
    time_t mtime;                 /* dir's modification time */
    struct t_tree *files;
    char name[];
  };

// in searc_directory():

      if (!*p) {                          /* if dir isn't in dir tree, add him */
        *p = malloc(sizeof(struct t_dirs) + strlen(dir) + 1);
        (*p)->left = (*p)->right = NULL;
        (*p)->files = NULL;
        strcpy((*p)->name, dir);
      }

This fixes the bug. Now name is used as it was defined. I think the original code tried to use the name variable as the 1st character of the string in order to avoid adding 1 in the malloc() call.

It's not a great solution though. It makes assumptions about how structures are allocated. Maybe it's valid to assume name occurs at the end of the structure-- it's been a long time since I looked at that. If I define name as a pointer and actually allocate it, then I don't care how structures are allocated.

[bob]

But how about this?

Code: Select all

 struct t_tree {
    struct t_tree *left, *right;
    char name[];
  };
  
  struct t_dirs {
    struct t_dirs *left, *right;
    time_t mtime;                 /* dir's modification time */
    struct t_tree *files;
    char name[];
  };

// in searc_directory():

      if (!*p) {                          /* if dir isn't in dir tree, add him */
        *p = malloc(sizeof(struct t_dirs) + strlen(dir) + 1);
        (*p)->left = (*p)->right = NULL;
        (*p)->files = NULL;
        strcpy((*p)->name, dir);
      }

This fixes the bug. Now name is used as it was defined. I think the original code tried to use the name variable as the 1st character of the string in order to avoid adding 1 in the malloc() call.

It's not a great solution though. It makes assumptions about how structures are allocated. Maybe it's valid to assume name occurs at the end of the structure-- it's been a long time since I looked at that. If I define name as a pointer and actually allocate it, then I don't care how structures are allocated.

By ansi C standards, a structure MUST have variables in the order given. Only freedom is dealing with alignment on machines where alignment is an issue. The reason is, without such a guarantee, how could you write a file from a C program and read it using a non-C program?