0
\$\begingroup\$

If used right, strcmp is not dangerous. However the code calling it might be. Limiting the opportunity for bugs I might in large teams "dictate" some kind of preemptive strike against bugs from bad calling code. It seems problem number one is comparing not zero-terminated strings. The core of my solution so far is this:

 // ----------------------------------------------
 // 
 #include<assert.h>
 #include<stdio.h>
 #include<stdlib.h>
 #include<string.h>

   #define VERIFY(X) do { if (!(X)) { perror(#X); exit(0); } } while(0)

   #define B(X_) (X_) ? "true" : "false"
   #define P(F,X_) fprintf( stdout, "\n%4d : %16s : " F, __LINE__, #X_, (X_))

   #undef COUNT_OF
   #define COUNT_OF(x) ((sizeof(x)/sizeof(0[x])) / ((size_t)(!(sizeof(x) % \
         sizeof(0[x])))))

// safer strcmp
static int safer_strcmp_ (
const size_t N, 
const size_t M, 
const char (*L)[N], 
const char (*R)[M])
{
 VERIFY( L );
 VERIFY( R );
 VERIFY( 0 == ((*L)[N]) );
 VERIFY( 0 == ((*R)[M]) );
    const char * str1 = &(*L)[0] ;
    const char * str2 = &(*R)[0] ;
            VERIFY( str1 );
           VERIFY( str2 );

    // https://codereview.stackexchange.com/a/31954/179062
    while (*str1 && *str1 == *str2) {
        str1++;
        str2++;
     }
       return *str1 - *str2;
  } // eof safer_strcmp_ 

 #undef  COMPARE
  // macro front is OK for string literals
  // COUNT_OF includes the EOS char, hence the -1
  #define COMPARE(A,B) \
   safer_strcmp_( COUNT_OF(A) - 1 ,COUNT_OF(B) - 1, &A, &B )

 int main(void) {

    // does not compile: P("%d", COMPARE( NULL,"")) ;
    P("%d", COMPARE("","")) ;
    P("%d", COMPARE("A","AA")) ;
    P("%d", COMPARE("A","A")) ;
    P("%d", COMPARE("AA","A")) ;
    P("%d", COMPARE("B","A")) ;

    return 0 ;
 }

https://godbolt.org/z/6bbeWazYW

Basically, the code just checks if both strings are properly terminated. That is a macro for calling it with literals:

The macro hides the complexity of making that call. If using variables, anything I can think of involves using strlen / strnlen and that defeats the purpose.

Is there a way of using the above with variables?

\$\endgroup\$
8
  • \$\begingroup\$ This looks more like a question of how to do something than a request for a code review. Consider asking this on StackOverflow instead. \$\endgroup\$ – G. Sliepen Apr 1 at 15:00
  • 3
    \$\begingroup\$ Are you targetting C99 or C11? You can't target both at the same time. \$\endgroup\$ – Mast Apr 1 at 18:32
  • \$\begingroup\$ @Mast you are right and I am not targeting them both in the same time. These are just tags on this post. Please see the Godbolt link provided for a working sample. \$\endgroup\$ – DBJDBJ Apr 1 at 19:21
  • \$\begingroup\$ Arthur, that is a Chromium macro \$\endgroup\$ – DBJDBJ Apr 1 at 19:57
  • 1
    \$\begingroup\$ After you get answers you are not supposed to edit the question, especially the code. Please (re)read What should I do when someone answers my question?. Every time you edit the code it shows up in chat and we need to review the edit. Changing the question after you have answers invalidates the answers. The code must remain the same as what the reviewers that posted answers saw or their answers don't make sense. \$\endgroup\$ – pacmaninbw Apr 8 at 19:09
4
\$\begingroup\$

Subtle functional bug

return *str1 - *str2; may return the wrong sign value when the char values are negative.

C string library functions specify:

For all functions in this subclause, each character shall be interpreted as if it had the type unsigned char (and therefore every possible object representation is valid and has a different value). C17 § 7.24.1 3

A repaired compare below.

Pedantic: This code also properly handles rare non-2's complement and when CHAR_MAX > INT_MAX unlike OP's.

const unsigned char *u1 = (const unsigned char *) str1;
const unsigned char *u2 = (const unsigned char *) str2;
while (*u1 && *u1 == *u2) {
    u1++;
    u2++;
}
return (*u1 > *u2) - (*u1 < *u2);

What is a string?

OP reports "It seems problem number one is comparing not zero-terminated strings.". The C library defines string below. A string always contains a null character '\0'.


A string is a contiguous sequence of characters terminated by and including the first null character.

Of course, an array of characters may lack a null character and so this function may have value with those non-strings.


Calling COMPARE() with a pointer and not an array leads to troubles, unlike strcmp().

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the sober response. \$\endgroup\$ – DBJDBJ Apr 2 at 2:17
  • \$\begingroup\$ I reverted back to using strcmp() after all the checks are done. \$\endgroup\$ – DBJDBJ Apr 8 at 19:01
1
\$\begingroup\$

The macro is only safe when passed string literals, it is not safe when you pass it pointers to strings. In particular, given a char *s, the result of COUNT_OF(s) is 8 on 64-bit machines. This means that if you pass it pointers to strings shorter than 8 characters (including the NUL-byte), it will read past the end of the string. Given that the whole point of the excercise is to provide more safety than regular strcmp(), this macro fails to deliver the promise. The macro is also not very useful for string literals, as the compiler will already guarantee that the literals are NUL-terminated.

It is not possible to make this code work with variables. The only way to check the length of a string pointed to by a char * variable is to call strlen() on it, and that assumes that the string is properly NUL-terminated. The only way to make this work is to have the caller pass the length of the string, but this just shifts the burden to the caller.

The best strategy in my opinion is just to have the caller call strcmp() when it knows both arguments are proper NUL-terminated strings, and strncmp() if it has knowledge of the size of one or both of the strings.

\$\endgroup\$
1
  • \$\begingroup\$ It might seem like it is safe, but consider what happens if we pass in very large values of N and M, say SIZE_MAX - 1. It would then check the byte right before the string. What if it happens to contain a zero? And even if you don't allow values that would cause the pointer to wrap, consider that not all memory in the process's memory space might be validly mapped. I could give it an N that points to a valid piece of memory with a NUL-byte, but there might be invalid memory between that and the start of the string. \$\endgroup\$ – G. Sliepen Apr 8 at 20:21
1
\$\begingroup\$

Apologies for changing the code that was answered. Since a few days ago I have evolved :)

Yes, that function is ok and will work for any legal call and that macro will work only with two string literals, of course. The only change I want to add inside the function is to call strncmp() instead of reinventing it

// two char arrays comparison
 int safer_strcmp_ (
   const size_t N, const size_t M, 
  /* pointer to char array of length N*/
   const char (*L)[N], 
  /* pointer to char array of length M */
  const char (*R)[M])
 {
   VERIFY( L && R );
   VERIFY( N > 0 && M > 0 );
   // checking if both strings
   // are zero terminated
   VERIFY( 0 == ((*L)[N]) );
   VERIFY( 0 == ((*R)[M]) );

   return strncmp( *L, *R, MIN(N,M) );
 }

Inbuilt str(n)cmp() is certainly much faster vs. what any result of my re-invention might be.

Thank you all for your involvement and time spent.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.