INTERNAL: do_lqdetect_write method simple

[cheesecrust]

🔗 Related Issue

• https://github.com/jam2in/arcus-works/issues/621

⌨️ What I did

• key 를 만드는 로직과 키를 이어붙이는 부분을 간소화 시켰습니다.
• https://github.com/jam2in/arcus-works/issues/627
  위 이슈에 맞추어 입력 가능한 버퍼의 공간 보다 큰 입력이 들어왔을때에 처리를 포함하였습니다.
  • key를 입력하기 전에 offset 이 length 를 넘을 경우 이후에 snprintf 에 0을 넘겨 써지지 못하도록 하였습니다.
  • 마지막 까지 다 썻을때 offset이 전체 주어진 길이보다 크면 offset을 length - 1 로 옮기고 해당 자리에 줄바꿈 \n을 삽입하여 다음 명령을 받을 준비 합니다.

[jhpark816]

리뷰 완료

[jhpark816]

리뷰 완료

[jhpark816]

@cheesecrust rebase해 주세요.

[jhpark816]

아래와 같이 하시죠.

• length 값 조정
• indentation은 아래 코드와 같이

    gettimeofday(&val, NULL);
    ptm = localtime(&val.tv_sec);
    length = ((nsaved+1) * LQ_INPUT_SIZE);

    buffer->offset += snprintf(buffer->data + buffer->offset, length - buffer->offset,
                               "%02d:%02d:%02d.%06ld %s <%u> %s ",
                               ptm->tm_hour, ptm->tm_min, ptm->tm_sec, (long)val.tv_usec,
                               client_ip, arg->overhead, command_str[cmd]);
    buffer->keypos[nsaved] = buffer->offset;
    buffer->keylen[nsaved] = keylen;

    buffer->offset += snprintf(buffer->data + buffer->offset, length - buffer->offset,
                               "%s %s\n", keyptr, arg->query);
    lqdetect.arg[cmd][nsaved] = *arg;
    buffer->nsaved += 1;

[namsic]

keylen > 0 확인하는 이유가 무엇인가요?

[cheesecrust]

• 로그가 다 안 써졌을 경우(키, 쿼리 모두 포함)에 해당 순서의 keylen을 -1 로 초기화 시켜 넘쳤다는 것을 표현하고 있습니다.
• 완전하지 못한 라인에 대해서는 비교를 해서는 안된다고 생각하여 완벽하지 못한 라인을 넘어가기 위해 추가하였습니다.

[namsic]

snprintf에서 overflow 발생하는 경우에는 여기뿐만 아니라 lqdetect show 명령 등에서도 문제가 발생하게 됩니다.

아래 항목을 충분히 확인하고 공유해 주기 바랍니다.
이와 관련한 수정은 다른 PR로 처리하는 것이 나을 것 같습니다.

AS-IS

• lqdetect_write 도중 snprintf에서 overflow 발생할 경우 문제가 생길 수 있는 부분들
• 기존 코드 기준 overflow 발생하면 어떻게 동작하는지?

TO-BE

• 구현을 어떻게 수정해야 하는지 (여러 구현 방식이 있을텐데, 그 중 낫다고 생각하는 방식과 그 이유)
• 수정하면, 이전에 문제 발생했던 상황에서 어떻게 동작하게 되는지

[jhpark816]

아래와 같이 합시다.

static void do_lqdetect_write(char *client_ip, char *key,
                              enum lq_detect_command cmd, struct lq_detect_argument *arg)
{
    struct tm *ptm;
    struct timeval val;
    struct lq_detect_buffer *buffer = &lqdetect.buffer[cmd];
    uint32_t nsaved = buffer->nsaved;
    uint32_t length, keylen = strlen(key);
    char keybuf[251];
    char *keyptr = key;

    if (keylen > 250) { /* long key string */
        keylen = snprintf(keybuf, sizeof(keybuf), "%.*s...%.*s",
                          124, key, 123, (key+keylen-123));
        keyptr = keybuf;
    }

    if (is_command_duplicated(keyptr, keylen, cmd, arg) == true) {
        return;
    }

    gettimeofday(&val, NULL);
    ptm = localtime(&val.tv_sec);

    length = ((nsaved+1) * LQ_INPUT_SIZE);
    snprintf(buffer->data + buffer->offset, length - buffer->offset,
             "%02d:%02d:%02d.%06ld %s <%u> %s ",
             ptm->tm_hour, ptm->tm_min, ptm->tm_sec, (long)val.tv_usec,
             client_ip, arg->overhead, command_str[cmd]);
    buffer->offset += strlen(buffer->data + buffer->offset);
    buffer->keypos[nsaved] = buffer->offset;
    buffer->keylen[nsaved] = keylen;

    snprintf(buffer->data + buffer->offset, length - buffer->offset,
             "%s %s\n", keyptr, arg->query);
    buffer->offset += strlen(buffer->data + buffer->offset);
    lqdetect.arg[cmd][nsaved] = *arg;
    buffer->nsaved += 1;
}

[cheesecrust]

@jhpark816
snprintf 시 overflow가 발생하면 '\n'을 삽입해주는 코드도 필요할 것 같습니다.

snprintf(buffer->data + buffer->offset, length - buffer->offset,
             "%s %s\n", keyptr, arg->query);

위 부분에서 만일 overflow가 발생하게 된다면 '\n'이 입력되지 않아 그 다음 라인에 와야할 라인이 이어 붙게될것 입니다.
따라서 아래와 같이 하는것은 어떤가요?

    buffer->offset += snprintf(buffer->data + buffer->offset, length - buffer->offset, ...);
    buffer->offset += snprintf(buffer->data + buffer->offset, (length - buffer->offset) > 0 ? (length - buffer->offset) : 0, ...)
        if (buffer->offset >= length) {
        buffer->offset = length - 1;
        buffer->data[buffer->offset - 1] = '\n';
    }

[jhpark816]

@namsic 위의 코멘트를 리뷰해 주세요.

[namsic]

위 부분에서 만일 overflow가 발생하게 된다면 '\n'이 입력되지 않아 그 다음 라인에 와야할 라인이 이어 붙게될것 입니다. 따라서 아래와 같이 하는것은 어떤가요?

keypos와 keylen 관련 변경은 어떻게 되나요?

위의 코멘트를 리뷰해 주세요.

snprintf의 overflow 관련 변경은 논의가 길어질 것 같아 이번 PR에서 제외하고
다음 작업으로 이어서 처리했으면 좋겠는데, 이번 PR에서 함께 처리해야 하나요?

[jhpark816]

snprintf의 overflow 관련 변경은 논의가 길어질 것 같아 이번 PR에서 제외하고
다음 작업으로 이어서 처리했으면 좋겠는데, 이번 PR에서 함께 처리해야 하나요?

이번 PR에서는 동작의 정확성만 보장되면 됩니다.