Re: Review about this code ?

Дана 24/09/25 08:21PM, bilal@iscarioth.org написа:
> Sorry for the inconvenience, I wanted to know if this
> piece of code seemed to be correct for a OpenBSD developer ?

Dear Bilal,

I think that C code review is out of the scope of this list. You could
try at:

https://codereview.stackexchange.com/


For kernel developer style guide, see style(9).


My proverbial two cents:


> #define SAFE_FREE(addr)  do { \
>   if (addr) \
>     { \
>       free (addr); \
>       addr = NULL; \
>     } \
>   else \
>     { \
>       fprintf (stderr, " (double free inside %s:%s) at L%d\n", \
>        __FILE__, __func__, __LINE__); \
>     } \
>   }  while(0);

- The do { } while(0) idiom inside macros should not end with a
semicolon. The idea for the usage of such macros is to write
something like

SAFE_FREE(addr);

which if written like in your email would be expanded to

do {
// etc...
} while(0);;

- You don't need to check if addr is NULL. See the description of free
in free(3). Setting addr to NULL is sufficient to prevent "double
free". Of course, also keep in mind variable scope.


>       const size_t len = strlen (*s);

This depends on the context, but consider what would happen if *s
contains a sequence of chars which is not NUL-terminated (not a
string): disaster.


>       char *new_line = realloc (line, len + offset + 2);
>       if (new_line == NULL)
>       {
>           if (line != NULL)
>           {
>                 SAFE_FREE (line);
>           }
>           break;
>       }

Typically, if (re)allocation fails, you want to abort the program as
soon as possible. So something like this would be preferable:

char* new_line = realloc(line, len + offset + 2);
if (!new_line)
{
perror("realloc");
exit(errno);
}

What's more, in a recent discussion the case of failing memory
(re)allocation was explicitly mentioned as the one where freeing memory
is detrimental.


>       line = new_line;
>       memset (&line[offset], 0, len);
>       strcat (strcat (&line[offset], *s), " ");
>       offset += len + 1 ;

You should check out memccpy(3), very roughly said: if properly used it
is much safer than the alternatives, and doubles as strcat.