Re: Review about this code ?

On Wed, Sep 25, 2024 at 10:58:44PM +0200, Страхиња Радић wrote:
> Дана 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.
>

Yes and like most modern software it abuses the heap.

--Stephen