c++stack-overflowaddress-sanitizer

AddressSanitizer StackOverflow Error in LeetCode #1768 - Merged String Alternately


I'm currently trying LeetCode's Merge Strings Alternately problem. The description of this problem is given below:

You are given two strings word1 and word2. Merge the strings by adding letters in alternating order, starting with word1. If a string is longer than the other, append the additional letters onto the end of the merged string.

Return the merged string.

And one of the test case is this:

word1 = "rlvrpyrhcxbceffrgiy";
word2 = "ktqi";

When I try this test case on my computer (I'm using CLion 2022.3.3), it works fine, but when I try to run my solution and test this case on LeetCode, it gets a Runtime Error.

Here is my solution:

class Solution {
public:
    string mergeAlternately(string word1, string word2) {
        
        string merged;
        char *p1,*p2;

        string longstring;
        
        if(word1.length() >= word2.length()){
            longstring = word1;
        }
        else{
            longstring = word2;
        }

        for (int i = 0; i < longstring.length(); i++) {
            p1 = &word1[i];
            p2 = &word2[i];
        
            if(*p1 != '\0' && *p2 != '\0'){
                merged += *p1;
                merged += *p2;
            }
            else if(*p1 != '\0' && *p2 == '\0'){
                merged += *p1;
            }
            else if(*p1 == '\0' && *p2 != '\0'){
                merged += *p2;
            }
        }
    
        return merged;

    }
};

And this is the error which I'm getting on LeetCode:

=================================================================
==22==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fa4c5900390 at pc 0x559334397d01 bp 0x7ffebc3d6450 sp 0x7ffebc3d6448
READ of size 1 at 0x7fa4c5900390 thread T0
    #3 0x7fa4c7591d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
    #4 0x7fa4c7591e3f  (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
Address 0x7fa4c5900390 is located in stack of thread T0 at offset 144 in frame
  This frame has 3 object(s):
    [32, 33) 'ref.tmp'
    [48, 80) 'agg.tmp'
    [112, 144) 'agg.tmp8' <== Memory access at offset 144 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
Shadow bytes around the buggy address:
  0x7fa4c5900100: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7fa4c5900180: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7fa4c5900200: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7fa4c5900280: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x7fa4c5900300: f1 f1 f1 f1 01 f2 00 00 00 00 f2 f2 f2 f2 00 00
=>0x7fa4c5900380: 00 00[f3]f3 f3 f3 f3 f3 00 00 00 00 00 00 00 00
  0x7fa4c5900400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fa4c5900480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fa4c5900500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fa4c5900580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fa4c5900600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==22==ABORTING

Please explain why this is occurring.


Solution

  • If the strings are different lengths, then you'll run right off the end of one of them, because you're always using the value i to get a pointer into each string, where i iterates over the longest string.

    You might hit the NULL terminator in one string, but if you keep looping, you'll jump to the next byte in memory and enter the realm of undefined behavior.

    for (int i = 0; i < longstring.length(); i++) {
        p1 = &word1[i];
        p2 = &word2[i];
    
        // ...
    }
    

    You should really be iterating over the shortest of the two strings in one loop, and then have two more loops after that to copy the remaining characters.

    It is cleaner to do this using iterators, completely avoiding pointers:

    string merged;
    merged.reserve(word1.size() + word2.size());
    
    // Interleave characters
    string::const_iterator p1 = word1.cbegin(), p2 = word2.cbegin();
    while (p1 != word1.cend() && p2 != word2.cend()) {
        merged += *p1++;
        merged += *p2++;
    }
    
    // Append remaining characters
    merged.append(p1, word1.cend());
    merged.append(p2, word2.cend());