I keep getting "ERROR: AddressSanitizer: heap-buffer-overflow on address"

Question

I can't figure out this error.

Problem > Write a function to find the longest common prefix string amongst an array of strings. If there is no common prefix, return an empty string "".

char * longestCommonPrefix(char ** strs, int strsSize){
    int i = 1;
    int j = 0;
    int k = 1;
    int n = 0;
    
    if (strsSize == 0) return ("");
    if (strsSize == 1) return (strs[0]); 
    if (strsSize > 1 && strcmp(strs[0],"") == 0) return ("");
    
    char *s = calloc(strlen(strs[0]), sizeof(char));
    char *temp = calloc(strlen(strs[0]), sizeof(char));
    if (!s || !temp) return (0);
    
    // while (strs[0][j] != '\0') {
    //     temp[j] = strs[0][j];
    //     j++;
    // }
    strcpy(temp, strs[0]);
    
    while (k < strsSize) {
        j = 0;
        n = 0;
        memset(s, 0, strlen(s));
        while (strs[i][j]){
            if (temp[j] == strs[i][j]) {
                s[n] = strs[i][j];
                n++;
            }
            else if (temp[j] != strs[i][j]){
                break;
            }
            j++;
        }
        strcpy(temp, s);
        k++;
        i++;
    }
    return (s);
}

It works without any problem on my visual studio but when I submit this code to Leetcode, it always has that error message. I assume error occurs on either line 7 or 8 because it dose not show same error when I hide those two line of codes. Also, when I copy strs[0] string to temp using while loop, it works fine on both Leetcode and visual studio but it does not work when I use "scrcpy" function on Leetcode but again, it works on my visual studio.

Another minor problem. The reason that I use calloc to allocate the memory is because It shows same error message "Address sani~" when using malloc. It does not matter on visual studio. Both calloc and malloc work ok.

I've added an extra memory for NULL terminator.

char *s = malloc((strlen(strs[0]) + 1) * sizeof(char));

It seems alright in my head but It has same memory issue.

I've tried many different ways but I still have issue with the way I use malloc and clear a string.

int len = strlen(strs[0]);
char *temp = malloc((len+1) * sizeof(char)) 
//it still gives me a memory error. 

When I clear my string with memset, it returns a correct output but it does not return the right answer when I clear the string simply using NULL terminator. Is there a difference?

s[j] = '\0';
memset(s, 0, strlen(s));

Answer 1

The main difference between malloc and calloc is that malloc only reserves memory but doesn't modify its contents, whereas calloc reserves memory and zero-initializes every byte.

So, what's happening inside your loop where you begin copying string prefixes into s is you did this:

memset(s, 0, strlen(s));

Now, you thought that you just cleared the string, but at this point it's the first time you've done anything with s at all. So if s was allocated with malloc then calling strlen(s) could do anything. Very likely the uninitialized memory contents will not be a NUL-terminated string, and strlen will happily search past the end of that block of memory and into other parts of memory that your program did not request.

This is known as Undefined Behavior.

So, what can you do about it? Well, in the comments you tried this:

s[0] = '\0';

Great! But now every other byte of the string is not initialized. You're about to loop over strs[i] and copy stuff into s. The problem is that afterwards you never copied in a NUL-terminator, so once again you have an un-terminated string. And, to be specific, the place where that terminator would have gone is still uninitialized.

As I already said very early on in comments, all you need to do is write that terminator after copying. You don't really need two variables j and n because they're going to be the same value anyway. Let's just use j. It makes the code simpler.

j = 0;
while (strs[i][j] && temp[j] == strs[i][j])
{
    s[j] = strs[i][j];
    j++;
}
s[j] = '\0';

That's all. You have a terminated string. You didn't need to pre-initialize it, because you were about to overwrite that memory anyway. The important part is the line after the loop.

---

Now, let's think a bit about what you're really trying to achieve here.

The task is to search all strings in the array and find the longest common prefix. And looking at your code, not only is it doing too much work, it's actually not even achieving the required task. Hint: you never used the variable k.

So why not take a step back and think about it differently? Do you actually need to copy prefixes over and over? No, you don't. All you need to do is see how many characters they have in common. And, you don't even need to check all the characters -- you only need to check up to whatever maximum count you've already found.

// Find longest common prefix
int commonPrefixLength = strlen(strs[0]);
for (int i = 1; i < strsSize; i++)
{
    int p = 0;
    while (p < commonPrefixLength && strs[i][p] == strs[0][p]) ++p;
    commonPrefixLength = p;
}

// Copy prefix to a new string
char *commonPrefix = malloc(commonPrefixLength + 1);
memcpy(commonPrefix, strs[0], commonPrefixLength);
commonPrefix[commonPrefixLength] = 0;

That's all you need to do. You don't even need to check for the NUL terminator when searching the strings because you've already bounded the search by the longest prefix and it's guaranteed that if you encounter a NUL-terminator then it won't be equal to the character you're testing in strs[0]. Nice.

However we have another issue, and that is your function is not consistent about what it returns. You wrote some special cases that might return strs[0] or a string literal "". Otherwise it allocates new memory and returns that. This is a problem, because the caller does not know whether it is supposed to free this memory or not.

So you should decide. The options are:

1. Function either allocates or returns NULL -- caller always responsible for freeing memory.
2. Function never allocates -- returns NULL or modifies str[0] and returns that

If you choose 2, then instead of allocating and copying stuff after the loop, you'd just terminate the string. However, you can only do this if input array was not string literals:

strs[0][commonPrefixLength] = '\0';

So the safest choice is probably option 1. But here are both...

Option 1:

char * longestCommonPrefix(char ** strs, int strsSize)
{
    if (strsSize == 0)
        return NULL;

    int commonPrefixLength = strlen(strs[0]);
    for (int i = 1; i < strsSize; i++)
    {
        int p = 0;
        while (p < commonPrefixLength && strs[i][p] == strs[0][p]) ++p;
        commonPrefixLength = p;
    }

    char *commonPrefix = malloc(commonPrefixLength + 1);
    if (commonPrefix != NULL)
    {
        memcpy(commonPrefix, strs[0], commonPrefixLength);
        commonPrefix[commonPrefixLength] = 0;
    }
    return commonPrefix;
}

Option 2:

char * longestCommonPrefix(char ** strs, int strsSize)
{
    if (strsSize == 0)
        return NULL;

    int commonPrefixLength = strlen(strs[0]);
    for (int i = 1; i < strsSize; i++)
    {
        int p = 0;
        while (p < commonPrefixLength && strs[i][p] == strs[0][p]) ++p;
        commonPrefixLength = p;
    }

    strs[0][commonPrefixLength] = '\0';
    return strs[0];
}

Live demo: https://godbolt.org/z/bebs7zz6j