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