Suspicious of passing a pointer to a SINGLE CHARACTER to strcmp()

To all: You've confirmed my suspicions enough that I will probably be fixing it.


To grg99:  let me give a more realistic snapshot of the code (the first example
was a bit out of context, but this one is closer, altough I've massaged it to make
it non-proprietary).


typedef struct stru_name_struct { char lastnm[21], firstnm[17]; } NAME_STRUCT;

int do_something(NAME_STRUCT *pbuff)
{
    char *buff = (char *)pbuff;  // in effect this will point to the "lastnm"  field in pbuff, I think

    char ch;

   if (buff == NULL) // if we passed in a NULL pointer to pbuff)
       buff = &ch;

  if (!strncmp(buff, "-ABC", 4)) // ABC is a flag that means to invoke special handling
     do something

...
}

Basically if *pbuff is passed in as NULL, we need buff to be the empty string. This whole
thing became an  issue because we are in the midst of a 16-bit DOS to 32-bit NT port.
Under 16-bit, we could get away with BUFF being NULL in the strncmp(); however,
under 32-bit, passing NULL to strncmp() resulted in a critical error, so we had to
massage buff to represent a blank string.

Thanks. Hope the above is clear.

Yep, better rewrite the whole thing, it's pretty shaky code:

int do_something(NAME_STRUCT *pbuff)
{

   if ( pbuff != NULL) {
         if ( ! strncmp( pbuff->lastnm, "-ABC", 4) ) // ABC is a flag that means to invoke special handling
                     do something
   }
   else {  // pbuff is NULL, don't do the special checks at all!!! }
}

One more thing....maybe I'm not thinking clearly,
but  I'm wondering, too, about the statement:

char *buff = (char *)pbuff;  

where, to remind everyone, pbuff is of type NAME_STRUCT which is:

typedef struct stru_name_struct { char lastnm[21], firstnm[17]; } NAME_STRUCT;

What exactly is HAPPENING there?  Is buff being assigned to the ADDRESS of pbuff??
It just looks a little odd to me...how exactly is the memory for BUFF being allocated?

Thx

Hi Steve,

This last one is fine.  It's actually fairlly common to assign the address of a data element to a pointer of a different type.  In this case, you 'recast' the pointer contained in pbuff (tell the compiler that you're 'changing' the pointer type from NAME_STRUCT to char) and store it in buff.


Kent

char *buff = (char *)pbuff;  

OK, I think I see what my confusion was.  :)  Somehow I was thinking that
char *buff

was somehow DEREFERNCING the charpointer BUFF but of course in this case it is not, as
it's part of a declaration.

If I later said

*buff = 'A';

THAT  would be a case of DEREFERENCING, right?


PS- I'm raising the points here...as this question has become more involved and y'all
have put in time and effort... :)

Not exactly.  To dereference an item is to obtain the item via its address.

void PrintIt (*int   IntVar)
{
  printf ("The value is %d", *IntVar);
}


The reference to *IntVar in the printf() function is a dereference of the pointer.

Kent

I stand corrected. But the first example

char *buff = (char*) pbuff

is DEFINITELY NOT A DE-REFERENCE, right?

Hmm....  Language and context here.  In it's strictest sense, to deference a pointer means to get to the data via the pointer.

So storing data at a pointer isn't really dereferencing the pointer.  Though we might find folks that will argue otherwise.  :)


Kent

Correct.  You're simply storing the pointer at another location.  There is a recast there, which tells the compiler to "change the datatype", but since you're converting a pointer to a pointer, no actual machine code is generated!


Kent

>>Yep, better rewrite the whole thing, it's pretty shaky code:

int do_something(NAME_STRUCT *pbuff)
{

   if ( pbuff != NULL) {
         if ( ! strncmp( pbuff->lastnm, "-ABC", 4) ) // ABC is a flag that means to invoke special handling
                     do something
   }
   else {  // pbuff is NULL, don't do the special checks at all!!! }
}<<

Now, getting back to the "correction" needed to this flawed code:
wouldn't the following work?


typedef struct stru_name_struct { char lastnm[21], firstnm[17]; } NAME_STRUCT;

int do_something(NAME_STRUCT *pbuff)
{
    char *buff = (char *)pbuff;  // in effect this will point to the "lastnm"  field in pbuff, I think

    char* empty_buff = "";

   if (buff == NULL) // if we passed in a NULL pointer to pbuff
       buff = &empty_buff;

   if (!strncmp(buff, "-ABC", 4)) // ABC is a flag that means to invoke special handling
     do something

...
}

Now, wouldn't the above be OK?? I _want_ the if !strcmp clause to FAIL if NULL is passed in to
BUFF, and I think I can do so safetly since now, buff would be pointing to something legitimate
and initialized (i.e. empty_buff)

Or, I suppose I could init empty_buff as follows:
char empty_buff[2];
empty_buff[0] = 0;

Thanks

Yes, you could do it that way, that would be a good "patch".

But if you're allowed to actually dig into the code, versus just patching it,
there's no point in stuffing an empty string into the buffer when you know the test is going to fail.
Better to not do the test at all.

That will work, but there are a couple of refinements that would make it better.

empty_buffer should be defined in the globals block.  Perhaps qualified with 'static' and/or 'const'.

static const char *empty_buffer = "";



typedef struct stru_name_struct { char lastnm[21], firstnm[17]; } NAME_STRUCT;

int do_something(NAME_STRUCT *pbuff)
{
    char *buff = (char *)pbuff;  // in effect this will point to the "lastnm"  field in pbuff, I think

    if (buff == NULL) // if we passed in a NULL pointer to pbuff
    {
      do whatever you want to do with a NULL pointer
    }
    else if (!strncmp(buff, "-ABC", 4)) // ABC is a flag that means to invoke special handling
    {
      do something
    }
...
}

This approach is slightly better in that the code more clearly describes what you're doing and why.  If the error handler is the same for the NULL pointer condition and the strncmp() failure, your approach may well be better.

Kent

"do whatever you want with a NULL pointer"
...which I assume would include
buff = &empty_buffer
???

One last clarification, the line:

   char *buff = (char *)pbuff;  // in effect this will point to the "lastnm"  field in pbuff, I think

is a bit iffy, yes it's correct, but neither clear, nor safe if the struct gets changed.

Quite a bit better would be:

   char *buff = pbuff->lastnm;  // Clearly point to the "lastnm"  field in pbuff  

OK, everyone.  I rewrote the code to use a real
string
char empty_buff[5];
rather than
a plain old
ch

and I make sure that empty_buff set set to be an empty
string.

Then when i assign
buff = &empty_buff
I think I'm OK.

Thanks for all the help, you confirmed my instincts
that something was not right :)