Expect buffer is (still) truncated

[Kris Gybels]

Could I ask you to check the code of _escapeZeroInExpectBuffer? Specifically:

https://bitbucket.org/tildeslash/monit/src/da9e25f5e8e1fd6cfd5aa74fdd4534af9be15065/src/protocols/generic.c#lines-42:54

I could be mistaken as my C is a little rusty, but there seems to be an error in the function as the output from the following is a\0\0b rather than a\0\0bc:

int main(int argc, char *argv[]) {
    char *buf = calloc(100, 1);
    buf[0] = 'a';
    buf[1] = '\0';
    buf[2] = '\0';
    buf[3] = 'b';
    buf[4] = 'c';
    _escapeZeroInExpectBuffer(buf, 100, 5);
    printf("%s\n", buf);
}

If I modify the call of memmove in the function from memmove(buf + j + 1, buf + j, n - i++) to memmove(buf + j + 1, buf + j, n - i) then the output from the above is a\0\0bc as expected.

I’ve attached complete code samples for reference, test1.c uses _escapeZeroInExpectBuffer without the modification to the memmove call and test2.c uses it with the modification.

Related issue: #444 ("expect buffer is truncated").

[Kris Gybels]

I see now that the converse modification of the call of memmove was done in commit 1924bfc to handle issue #1005. I think the correct fix for that issue would actually be memmove(buf + j + 1, buf + j, MIN(n - i, buflen - j - 1)) no?

[Kris Gybels]

I gave it some further thought, here’s an alternative implementation for the escape function:

void escape(char *buf, int buflen, int n) {
    int i, j;
    char c;
    for (i = 0, j = 0; i < n && j < buflen; i++, j++) {
        if (buf[i] == '\0') {
            if (++j == buflen) {
                j -= 1;
                break;
            }
        }
    }
    buf[j] = '\0';
    if (j == i)
        return;
    for ( ; i > 0; ) {
        c = buf[--i];
        if (c == '\0') {
            buf[--j] = '0';
            buf[--j] = '\\';
        } else {
            buf[--j] = c;
        }
    }
}

The following can be used to test the escape function:

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

void escape(char *buf, int buflen, int n) {
    exit(2); // See above for actual implementation
}

int read(char *buf, char *input) {
    char c;
    for (int i = 0; ; i++) {
        c = *(input++);
        if (c == '\0')
            return i;
        *(buf++) = (c == '.') ? '\0' : c;
    }
}

void test(char *input, int buflen, char *expected) {
    char *buf = malloc(buflen + 1);
    int n = read(buf, input);
    escape(buf, buflen, n);
    int ok = strcmp(buf, expected) == 0;
    printf("Input   : %s\nBuflen. : %i\nEscaped : %s\nExpected: %s\n%s\n\n",
        input, buflen, buf, expected, ok ? "OK" : "FAILED");
    if (!ok)
        exit(1);
    free(buf);
}

int main(int argc, char *argv[]) {
    test("", 0, "");
    test("a", 1, "a");
    test("a", 2, "a");
    test(".", 1, "");
    test(".", 2, "\\0");
    test("a..bc..", 7, "a\\0\\0bc");
    test("a..bc..", 8, "a\\0\\0bc");
    test("a..bc..", 9, "a\\0\\0bc\\0");
    test("a..bc..", 10, "a\\0\\0bc\\0");
    test("a..bc..", 11, "a\\0\\0bc\\0\\0");
    return 0;
}

‌

[Tildeslash]

Fixed: Issue #1029: The generic protocol test truncated the received data in the case that it contained zeros. The problem is a regression of 5.31.0 ... we need to make sure that we don't memmove data past the buffer boundary ... typically when we received more data then the "Run.limits.sendExpectBuffer" limit, the 'buflen' and 'n' matched, so there was no space for moving bytes that follow the one being escaped (which was the root cause of #1005) => we have to start truncating the data. The fix for #1005 was bad though, as it truncated the source data by one after each escaping, even if there was space in the buffer.

→ <<cset b4d2b6a4854b>>

[Tildeslash]

Thank you for excellent bug report, the problem is fixed

[Kris Gybels]

No problem! Could it be that the implementation in commit b4d2b6a4854b still has a bug though? For the following test:

test("a..bcd.", 11, "a\\0\\0bcd\\0");

If I use the implementation of the escape function I gave above, I get:

Input   : a..bcd.
Buflen. : 11
Escaped : a\0\0bcd\0
Expected: a\0\0bcd\0
OK

But if I use the implementation from commit commit b4d2b6a4854b, I get:

Input   : a..bcd.
Buflen. : 11
Escaped : a\0\0bcd\00
Expected: a\0\0bcd\0
FAILED

‌

[Tildeslash]

the test with “a..bcd.” works for me, a test program:

‌

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


#define MIN(x,y) ((x) < (y) ? (x) : (y))


// Escape binary 0 as "\0" pair
static char *_escapeZeroInExpectBuffer(char *buf, int bufferLength, int contentLength) {
        int currentByteIndex = 0;
        for (int bytesProcessed = 0; bytesProcessed < contentLength && currentByteIndex < bufferLength; bytesProcessed++, currentByteIndex++) {
                if (buf[currentByteIndex] == '\0') {
                        // Escape the zero, unless we run out of space in the buffer
                        if (currentByteIndex + 1 < bufferLength) {
                                // Shift the remaining content by one to the right, to make space for '\'. If there's no space for all remaining bytes, we'll truncate the data
                                memmove(buf + currentByteIndex + 1, buf + currentByteIndex, MIN(contentLength - bytesProcessed, bufferLength - currentByteIndex - 1));
                                // Escape 0 with "\0"
                                buf[currentByteIndex] = '\\';
                                buf[currentByteIndex + 1] = '0';
                                currentByteIndex++;
                        }
                }
        }
        return buf;
}

int main(int argc, char *argv[]) {
    char *buf = calloc(100, 1);
    buf[0] = 'a';
    buf[1] = '\0';
    buf[2] = '\0';
    buf[3] = 'b';
    buf[4] = 'c';
    buf[5] = 'd';
    buf[6] = '\0';
    _escapeZeroInExpectBuffer(buf, 100, 7);
    printf("%s\n", buf);
}

Output:

$ ./test1 

a\0\0bcd\0

I think the problem could be the malloc() in the test() function, which doesn’t initialize the buffer … and as the _escapeZeroInExpectBuffer() doesn’t make sure the buffer is NULL terminated on return (the caller does this by allocating extra byte and using calloc), there was probably some garbage.

[Kris Gybels]

Ah right, I didn’t take into account that I handled the null terminator a bit differently in my implementation of escape.

However, if I understand the code of ‘check_generic’ correctly, the buffer can get reused, or is that not the case? In the following, the buffer is reused; I get \0bd on the second output line rather than just \0b as I would expect:

// Base code: <https://bitbucket.org/tildeslash/monit/issues/1029/expect-buffer-is-still-truncated#comment-61854369>
// Replace main by:
int main(int argc, char *argv[]) {
    int buflen = 100;
    char *buf = calloc(buflen + 1, 1);
    buf[0] = 'a';
    buf[1] = 'b';
    buf[2] = 'c';
    buf[3] = 'd';
    buf[4] = '\0'; // set terminator
    _escapeZeroInExpectBuffer(buf, buflen, 4); // 4 bytes read
    printf("%s\n", buf);
    buf[0] = '\0';
    buf[1] = 'b';
    buf[2] = '\0'; // set terminator
    _escapeZeroInExpectBuffer(buf, buflen, 2); // 2 bytes read
    printf("%s\n", buf);
}

To handle that, I think the call of memmove in _escapeZeroInExpectBuffer should move one more byte so that it also moves the terminator?

[Tildeslash]

you are absolutely right, it is reused … although the last \0 is still in place (the buflen passed to _escapeZeroInExpectBuffer doesn’t include it), the nul terminator which was set past the Socket_read can be smashed by the escaping and the content of previous response (provided it was longer) may show up in the buffer … will fix

[Tildeslash]

fixed: https://bitbucket.org/tildeslash/monit/commits/4ca16fdb75d62d2a52c68f05a56320e66675908d

‌

[Kris Gybels]

Ok great, thanks!

[Kris Gybels]

Just as a note to myself for future reference: in the version of escape I gave above, the second loop unnecessarily traverses the part of the buffer before the first zero byte (if there is one). The check j == i should actually be the end condition of the second loop, so:

void escape(char *buf, int buflen, int n) {
    int i, j;
    char c;
    for (i = 0, j = 0; i < n && j < buflen; i++, j++) {
        if (buf[i] == '\0') {
            if (++j == buflen) {
                j -= 1;
                break;
            }
        }
    }
    buf[j] = '\0';
    while (!(j == i)) {
        c = buf[--i];
        if (c == '\0') {
            buf[--j] = '0';
            buf[--j] = '\\';
        } else {
            buf[--j] = c;
        }
    }
}

‌