Garbage value with the string comparison

Question

#include<stdio.h>
#include<string.h>

void* lsearch(void* key,void* base,int size,int elemSize){
  int i = 0;
  for(i = 0; i < size; i++){
    void* elemAddr = ((char*)base + (i * elemSize));
    if(memcmp(key,elemAddr,elemSize)==0){
            return elemAddr;
    }
  }
  return NULL;
}

int main(){
  char* a[] = {"Hello","Good","Morning","Ladies"};
  int size = sizeof(a)/sizeof(a[0]);
  char* key = "Good";
  char* search = (char*)lsearch(&key,&a,size,sizeof(char*));
  if(search == NULL){
   printf("\n key value not found!! \n");
   return -1;
  }
  printf("\n search : %s \n",search);
  return 0;
}

OUTPUT:

 search : �@

I'm trying to do a lsearch on the strings in an array... The string matches, but I get a garbage value printed.. Why is it so..

1) Do not cast `void *` in C! 2) Do not use `void *` where you have defined types, unless you hate your compiler or prefer debugging code due to type errors the compiler can easily report! — too honest for this site, Oct 22 '15 at 21:24
I continue: do not do arithmetic on `void*` this is not well defined. Your call to your search function is bogus, it has too many `&` operators — Jens Gustedt, Oct 22 '15 at 21:26
Even more problems: `lsearch` assumes every element of the array is of equal size, but the strings in `a` aren't. Moreover: `lsearch` is comparing pointers to the string literals, rather than the strings themselves. — Kninnug, Oct 22 '15 at 21:29
@OLAF: I thought of having a generic function to be used and hence i used void* — Angus, Oct 22 '15 at 21:31
You already cast to `char *`. Also, as @JensGustedt wrote, arithmetic on pointers is undefined behaviour. Actually, your compiler should have warned you. And as you see, it is a nightmare for debugging. How could that be generic anyway? C does not have dynamic typing. Briefly: **do not!** — too honest for this site, Oct 22 '15 at 21:37
And do not return a negative errorcode from `main` unless it is a system error code. — too honest for this site, Oct 22 '15 at 21:42

maddouri · Accepted Answer · 2015-10-22T22:13:19.760

Fixed:

A few things were wrong, here are the most notable issues:

arithmetic on a void*: illegal,
the length of a some_type array[] = {...}; is sizeof(array) (no fancy divisions whatsoever),
inappropriate choice of interfaces: when you're just reading, consider using const parameters. Also, make sure the parameters' types are consistent with your arguments (e.g. base is a const char* [] and NOT a char*). Finally, to look for a key in an array of strings you need a key, a handle to the array and the length of the array, that's all.

Run It Online

#include<stdio.h>
#include<string.h>

const char* lsearch(const char* key, const char* base[], const size_t size){
  size_t i = 0;
  const size_t max_len = strlen(key);  // strlen(), that's how you compute the length of a string (you might add `+1` to include `\0` if you want)

  for(i = 0; i < size; i++){
    const char* elemAddr = base[i];  // Arithmetic on a void* is illegal in both C and C++: https://stackoverflow.com/a/3524270/865719
    if(memcmp(key, elemAddr, max_len)==0) {  // @TODO use strncmp(), more appropriate, and safer. In particular, if strlen(elemAddr) < strlen(key)
            return elemAddr;
    }
  }
  return NULL;
}

int main() {
  // init
  const char* a[]   = {"Hello","Good","Morning","Ladies"};
  const size_t size = sizeof(a);  // size of the array -- i.e. element count

  // search
  const char* key = "Morning";
  const char* search = lsearch(key, a, size);

  // results
  if(search == NULL) {
   printf("\n key value not found!! \n");
   return -1;
  }

  printf("\n search : %s \n",search);
  return 0;
}

As pointed out by Jongware, a better way would be to use a string comparison function. In my opinion, the safest bet is

int strncmp(const char *s1, const char *s2, size_t n);

And you would use it as such:

// safer. in particular, if (and when) strlen(elemAddr) < strlen(key)
if(strncmp(key, elemAddr, max_len) == 0) {
        return elemAddr;
}

Are you sure you are not comparing addresses here? A proper string comparison should use `strcmp`. Done properly, it would need a `compareXX` callback of the form `int (*compareFunction)(void *, void *)` — Jongware, Oct 22 '15 at 22:00
I agree :) Just edited the answer to mention `strncmp()`, which should be enough for such a simple example. — maddouri, Oct 22 '15 at 22:07

score 0 · Answer 2 · answered Oct 22 '15 at 21:36

0

That would do it:

  printf("\n search : %s \n",*(char**)search);

You were trying to read from the right location but the compiler didn't know how to access it

answered Oct 22 '15 at 21:36

CIsForCookies

12,097
11
59
124

1

This is a typical example of shooting Elephants in C (using wild pointers). Just casting is not how to solve it, but getting the types right and not using `vloid *` everywhere. No wonder this gets messed up.. – too honest for this site Oct 22 '15 at 21:39
It isn't great, I agree, but that's a fix. I assume here there are no mistakes in where the pointers point (and in this specific code the pointers are correct) – CIsForCookies Oct 22 '15 at 21:41
1

`search` has already a type. If that is not the same returned from `lsearch`, there is something wrong with that function or the type of `search` has to be fixed. But not just cast an argument to `printf`. This is a nightmare of maintenance. Like painting a drowning Submarine. – too honest for this site Oct 22 '15 at 21:44

Garbage value with the string comparison

2 Answers2