Learn how to a build a cloud-first strategyRegister Now

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 342
  • Last Modified:

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

I encountered some code as follows:

...
char oldbuff[20];
char *buff;  // a charpointer
char ch;

if (some condition)
  buff = &ch; // sets BUFF to the address of ch
else
  strcpy(buff, oldbuff);  // assume OLDBUFF contains valid string

..and then a bit later on...

if strcmp(buff, "ABC")
   do something

In the case where buff was set to &ch, I am very suspicous
of the above strcmp. Since buff points to a single CHARACTER,
rather than a string, I think it's asking for trouble.

Not to mention that ch is also  unitialized, but even if ch
WERE initialized, I still object to the  strcmp().

Would anyone agree that the above code is flawed?

Thanks


0
Stephen Kairys
Asked:
Stephen Kairys
  • 7
  • 6
  • 4
  • +2
4 Solutions
 
Kent OlsenData Warehouse Architect / DBACommented:
Hi stevefromc,

Good observations.  Though odd, the author could be using ch as an empty string sentinel.

If the variables are in the globals block, they are, by default, initialized to zero.  In this case, buff=&ch is a VERY ODD way of setting buff to an empty string.

If the variables are on the stack (local to some function) the results are undefined and this code is subject to odd failures.

In either case, it's a very bad coding technique.



Kent
0
 
brettmjohnsonCommented:
strcmp() compares two sequences of char, each terminated with a NUL byte.
So you are correct in your suspicion.  Passing a pointer to single char buffer to strcmp()
would only be valid if that char was '\0' (NUL).   And then, it would only match another
empty string.

0
 
PaulCaswellCommented:
>>Would anyone agree that the above code is flawed?
Not necessarily flawed, just dangerous. If it is possible to prove that if your 'some condition' is true then either the 'strcmp' will NEVER be called or 'ch' will ALWAYS contain a '\0' then the code stands as valid.

I have to admit that if I saw this code in software I was trying to debug I would change it to use something like 'char * empty = "";' instead of 'char ch = '\0';' but that was not the question.

Paul
0
What does it mean to be "Always On"?

Is your cloud always on? With an Always On cloud you won't have to worry about downtime for maintenance or software application code updates, ensuring that your bottom line isn't affected.

 
grg99Commented:
Wait a minute, there's SEVERAL things wrong with this code:

Case 1:

if "some condition" is TRUE:

  buff = &ch; // sets BUFF to the address of c..

if strcmp(buff, "ABC")
   do something

... strcmp is going to try to compare the string at "ch" with "ABC".
Now depending on the whims of the compiler, "ch" may be allocated as a single byte, or as a single byte, padded either before or after or both with 1,3,7, or 15 bytes, in order to align it on a machine-convenient boundary.  Now the code doesnt indicate if these varaibles are globals or locals (MOST PROBABLY LOCALS), so we don't know if those padding bytes are going to be filled with zero (if globals) or random garbage (if locals).   If they're filled with zeroes, the code has a CHANCE of  working, if its filled with garbage, then strcmp is likely to return "No match", as eventually it's going to hit a zero byte somewhere, most likely around 128 bytes past ch.  This won't cause any MAJOR problems, unless strcmp falls off the end of the stack or data segment before finding a zero byte, in which case you could get a hard crash with an addrssing error.

------------------------------
case 2:

If "some condition" is false, then the code is even more puzzling as " strcpy(  buff, oldbuff )" is going to try to move the string at oldbuff (which you claim contains a good string), to the address pointed to by "buff" (which you havent indicated what's in at that instant).

I have to assume if this code has EVER worked, that you havent shown us some initialization line that sets "buff" to point to some valid memory area that can hold oldbuff.

-----------------------

How about you show us ALL of the code?    There's still a small chance it's okay in at least one of those cases.







0
 
Stephen KairysTechnical Writer - ConsultantAuthor Commented:
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.

0
 
grg99Commented:
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!!! }
}


0
 
Stephen KairysTechnical Writer - ConsultantAuthor Commented:
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




0
 
Kent OlsenData Warehouse Architect / DBACommented:

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
0
 
Stephen KairysTechnical Writer - ConsultantAuthor Commented:
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... :)
0
 
Kent OlsenData Warehouse Architect / DBACommented:

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
0
 
Stephen KairysTechnical Writer - ConsultantAuthor Commented:
I stand corrected. But the first example

char *buff = (char*) pbuff

is DEFINITELY NOT A DE-REFERENCE, right?
0
 
Kent OlsenData Warehouse Architect / DBACommented:

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
0
 
Kent OlsenData Warehouse Architect / DBACommented:

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
0
 
Stephen KairysTechnical Writer - ConsultantAuthor Commented:
>>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




0
 
grg99Commented:
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.

0
 
Kent OlsenData Warehouse Architect / DBACommented:

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
0
 
Stephen KairysTechnical Writer - ConsultantAuthor Commented:
"do whatever you want with a NULL pointer"
...which I assume would include
buff = &empty_buffer
???
0
 
grg99Commented:
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  
0
 
Stephen KairysTechnical Writer - ConsultantAuthor Commented:
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 :)
0

Featured Post

Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

  • 7
  • 6
  • 4
  • +2
Tackle projects and never again get stuck behind a technical roadblock.
Join Now