Skip to main content
Code Review

Return to Revisions

2 of 3
C still allows non-const string literals
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
static void exit_with_message(char *func_name, char *ptr_name, char *message)
{
 printf("(%s)-(%s): %s\n", func_name, ptr_name, message);
 exit(1);
}

We should annotate this function with [[noreturn]].

The arguments should be const char* so that we can call with string literals (as indeed we do) without swamping us with compiler warnings (if you've not enabled such warnings, I recommend you do - they can catch serious bugs).

Error messages should go to standard error stream:

 fprintf(stderr, "(%s)-(%s): %s\n", func_name, ptr_name, message);

If we include <stdlib.h>, we can use a more descriptive exit status:

 exit(EXIT_FAILURE);

void freestr(String *w)
{
 if (!w->s)
 return;
 free(w->s);
 if (w->s != NULL)
 w->s = NULL;
}

Both of the tests are unnecessary - if we remove them, the behaviour is unchanged. However, it may be a good idea to check that w is not null before accessing its members:

void freestr(String *w)
{
 if (!w) {
 return;
 }
 free(w->s);
 w->s = NULL;
 w->length = 0;
}

size_t i, j;
String newString;

Prefer to defer declaration of locals to the point where they can be initialised. It's no longer necessary to declare all variables at the start of their scope.

char *buff = malloc(sizeof(*buff) * (w1->length + w2->length + 1));

Multiplication by sizeof *buff is redundant given that sizeof (char) is necessarily 1.


After making the code const-correct, fix the memory leaks:

gcc-14 -std=c23 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wmissing-braces -Wconversion -Wstrict-prototypes -fanalyzer 293412.c -o 293412
293412.c: In function ‘test_newstr_length’:
293412.c:241:1: warning: leak of ‘<U7510>.s’ [CWE-401] [-Wanalyzer-malloc-leak]
 241 | }
 | ^
 ‘main’: events 1-2
 |
 | 374 | int main()
 | | ^~~~
 | | |
 | | (1) entry to ‘main’
 | 375 | {
 | 376 | tests();
 | | ~~~~~~~
 | | |
 | | (2) calling ‘tests’ from ‘main’
 |
 +--> ‘tests’: events 3-4

(much snipped to fit within max answer length)

 | | (245) following ‘false’ branch...
 |......
 | 214 | return 1;
 | | ~
 | | |
 | | (248) ...to here
 |
 <------+
 |
 ‘test_newstr_actual_string’: event 249
 |
 | 254 | assert(issame(newstr("").s, ""));
 | | ^
 | | |
 | | (249) returning to ‘test_newstr_actual_string’ from ‘issame’
 |
 ‘test_newstr_actual_string’: event 250
 |
 | 254 | assert(issame(newstr("").s, ""));
 | | ^~~~~~
 | | |
 | | (250) following ‘false’ branch...
 |
 ‘test_newstr_actual_string’: events 251-252
 |
 | 255 | }
 | | ^
 | | |
 | | (251) ...to here
 | | (252) ‘<Uf630>.s’ leaks here; was allocated at (236)
 |
valgrind --leak-check=full ./293412 
==2357414== Memcheck, a memory error detector
==2357414== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==2357414== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==2357414== Command: ./293412
==2357414== 
----- STRING TESTS PASS ------
==2357414== 
==2357414== HEAP SUMMARY:
==2357414== in use at exit: 1,243 bytes in 20 blocks
==2357414== total heap usage: 2,047 allocs, 2,027 frees, 504,947 bytes allocated
==2357414== 
==2357414== 1 bytes in 1 blocks are definitely lost in loss record 1 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109BE6: test_newstr_actual_string (293412.c:254)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 4 bytes in 1 blocks are definitely lost in loss record 2 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x1093AD: concatRaw (293412.c:120)
==2357414== by 0x109EE5: test_concatRaw (293412.c:302)
==2357414== by 0x10A010: test_concat (293412.c:325)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 6 bytes in 1 blocks are definitely lost in loss record 3 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x1098A3: test_newstr_length (293412.c:235)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 6 bytes in 1 blocks are definitely lost in loss record 4 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x1098E0: test_newstr_length (293412.c:236)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 6 bytes in 1 blocks are definitely lost in loss record 5 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x10991D: test_newstr_length (293412.c:237)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 6 bytes in 1 blocks are definitely lost in loss record 6 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x10995A: test_newstr_length (293412.c:238)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 6 bytes in 1 blocks are definitely lost in loss record 7 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109A18: test_newstr_actual_string (293412.c:246)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 6 bytes in 1 blocks are definitely lost in loss record 8 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109AFF: test_newstr_actual_string (293412.c:249)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 7 bytes in 1 blocks are definitely lost in loss record 9 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109AB2: test_newstr_actual_string (293412.c:248)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 7 bytes in 1 blocks are definitely lost in loss record 10 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109B4C: test_newstr_actual_string (293412.c:250)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 7 bytes in 1 blocks are definitely lost in loss record 11 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109B99: test_newstr_actual_string (293412.c:251)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 14 bytes in 1 blocks are definitely lost in loss record 12 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109997: test_newstr_length (293412.c:239)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 14 bytes in 1 blocks are definitely lost in loss record 13 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x1093AD: concatRaw (293412.c:120)
==2357414== by 0x109DFD: test_concatRaw__basic (293412.c:288)
==2357414== by 0x10A006: test_concat (293412.c:323)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 21 bytes in 1 blocks are definitely lost in loss record 14 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x10A2F0: test_charat (293412.c:354)
==2357414== by 0x10A412: tests (293412.c:370)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 22 bytes in 1 blocks are definitely lost in loss record 15 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x1099D4: test_newstr_length (293412.c:240)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 22 bytes in 1 blocks are definitely lost in loss record 16 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x109D00: test_concat__basic (293412.c:275)
==2357414== by 0x10A001: test_concat (293412.c:322)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 27 bytes in 1 blocks are definitely lost in loss record 17 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109A65: test_newstr_actual_string (293412.c:247)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 30 bytes in 1 blocks are definitely lost in loss record 18 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x109C81: test_concat__basic (293412.c:271)
==2357414== by 0x10A001: test_concat (293412.c:322)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 30 bytes in 1 blocks are definitely lost in loss record 19 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x109D7F: test_concat__basic (293412.c:279)
==2357414== by 0x10A001: test_concat (293412.c:322)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== 1,001 bytes in 1 blocks are definitely lost in loss record 20 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x1093AD: concatRaw (293412.c:120)
==2357414== by 0x109FAC: test_concatRaw__stress (293412.c:314)
==2357414== by 0x10A00B: test_concat (293412.c:324)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414== 
==2357414== LEAK SUMMARY:
==2357414== definitely lost: 1,243 bytes in 20 blocks
==2357414== indirectly lost: 0 bytes in 0 blocks
==2357414== possibly lost: 0 bytes in 0 blocks
==2357414== still reachable: 0 bytes in 0 blocks
==2357414== suppressed: 0 bytes in 0 blocks
==2357414== 
==2357414== For lists of detected and suppressed errors, rerun with: -s
==2357414== ERROR SUMMARY: 20 errors from 20 contexts (suppressed: 0 from 0)

And everything in Loki's answer.

Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
default

AltStyle によって変換されたページ (->オリジナル) /