Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

INTERNAL: Fix an overwrite issue for the big key #338

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

INTERNAL: Fix an overwrite issue for the big key #338

wants to merge 1 commit into from
+47 −30

Conversation

ing-eoking
Copy link
Collaborator

@ing-eoking ing-eoking commented Jan 16, 2025 •
edited
Loading

🔗 Related Issue

⌨️ What I did

  • fetch 연산에서 키가 MEMCACHED_MAX_KEY보다 클 경우, 덮어쓰기가 발생하지 않도록 방지합니다. 만약 덮어쓰기가 발생하면 이를 로그에 기록합니다.
    • OVERWRITE가 발생한 경우, 사용자는 키 길이만으로 이를 충분히 감지할 수 있을 것으로 보입니다.
  • Binary 프로토콜 관련 부분도 수정했습니다.
    • Binary 프로토콜에서는 키 길이를 알 수 있기 때문에, MEMCACHED_MAX_KEY만큼 데이터를 저장한 후 나머지 데이터는 임시 버퍼로 flush 처리했습니다.

@ing-eoking ing-eoking marked this pull request as draft January 16, 2025 01:14
@ing-eoking ing-eoking force-pushed the bigkey branch 4 times, most recently from e7b429e to 5092df5 Compare January 16, 2025 03:21
@ing-eoking ing-eoking marked this pull request as ready for review January 16, 2025 03:24
@ing-eoking
Copy link
Collaborator Author

ing-eoking commented Jan 16, 2025 •
edited
Loading

@jhpark816

수정완료되었습니다.

CI 테스트가 실패하여 잠시 draft로 변경하겠습니다.

@ing-eoking ing-eoking marked this pull request as draft January 16, 2025 06:20
@ing-eoking ing-eoking marked this pull request as ready for review January 16, 2025 08:49
jhpark816
jhpark816 reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일부 리뷰

libmemcached/response.cc Show resolved Hide resolved
@jhpark816 jhpark816 requested a review from namsic January 17, 2025 04:13
@jhpark816
Copy link
Contributor

@namsic 리뷰 부탁합니다.

jhpark816
jhpark816 reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 완료

libmemcached/response.cc Show resolved Hide resolved
jhpark816
jhpark816 reviewed Jan 20, 2025
View reviewed changes
libmemcached/response.cc
rsize= keylen;
} else {
rsize= MEMCACHED_MAX_KEY - 1;
memcached_set_error(*ptr, MEMCACHED_KEY_TOO_BIG, MEMCACHED_AT);
Copy link
Contributor

@jhpark816 jhpark816 Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로, 이렇게 설정된 MEMCACHED_KEY_TOO_BIG 오류가 나중에 활용 되나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

활용되지 않으며, 로그를 남기기 위해 추가한 것이라 없어도 문제 없습니다. 제거할까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어떻게 하는 것이 좋은가요?
어떻게 해야 할지 모른다면, 현재는 그대로 두었다가 나중에 리뷰할 시에 살펴봐야 할 것 같습니다.

@ing-eoking ing-eoking mentioned this pull request Jan 21, 2025
Merged
@ing-eoking ing-eoking marked this pull request as draft January 21, 2025 03:25
@ing-eoking ing-eoking marked this pull request as ready for review January 23, 2025 08:16
@ing-eoking

This comment was marked as outdated.

Sign in to view
@ing-eoking
Copy link
Collaborator Author

@jhpark816

jhpark816
jhpark816 reviewed Feb 10, 2025
View reviewed changes
Copy link
Contributor

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 완료

libmemcached/response.cc
result->item_key[result->key_length]= 0;

if (key_length < MEMCACHED_MAX_KEY) key[key_length]= 0;
else key[MEMCACHED_MAX_KEY - 1]= 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문이 있습니다.

result->item_key 에는 최대 (MEMCACHED_MAX_KEY -1) 길이의 문자열을 담지만,
result->key_length는 MEMCACHED_MAX_KEY 이상일 수 있습니다.

result 구조체에 있는 item_key와 key_length 정보가 불일치하는 상태가 발생하며,
이로 인한 부작용은 없나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhpark816 jhpark816 jhpark816 left review comments

@namsic namsic Awaiting requested review from namsic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ing-eoking @jhpark816