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.

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

INTERNAL: Fix compile errors with zookeeper in redhat8 #283

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

ing-eoking
Copy link
Collaborator

@ing-eoking ing-eoking commented Jan 29, 2024

with-zk-integration 옵션을 넣고 compile 시, redhat8 계열에서 일어나는 컴파일 에러 수정입니다.

arcus/multi_process.c:105:7: error: ‘memset’ used with length equal to number of elements without multiplication by element size [-Werror=memset-elt-size]
       memset(errors, 0, NUM_OF_PIPED_ITEMS);
       ^~~~~~

libmemcached/arcus.cc:941:10: error: ‘char* strncpy(char*, const char*, size_t)’ specified bound 10240 equals destination size [-Werror=stringop-truncation]
   strncpy(buffer, serverlist, ARCUS_MAX_PROXY_FILE_LENGTH);
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

libmemcached/arcus.cc:584:32: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size between 232 and 237 [-Werror=format-truncation=]
   snprintf(path, sizeof(path), "%s/%s/%.*s_%s_%u_c_%s_%d%02d%02d%02d%02d%02d_%llx",
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

에러 원인 및 처리

  1. sprintf 수행 시 output 버퍼의 크기가 (input 버퍼의 크기 + 기타 문자열 크기) 보다 작음.
    • output 버퍼의 크기를 2배(512bytes) 늘렸습니다.
  2. strnpcy 수행 시 input 문자열의 크기가 output 버퍼보다 클 수 있음
    • 수행 이전에 비교 조건문을 추가했습니다.
    • 기존 코드에서 에러에 대한 별다른 핸들링이 존재하지 않아 똑같이 바로 return 하도록 했습니다.
      • 반복적으로 수행될 수 있는 코드이기에 별다른 에러 핸들링이 존재하지 않는 것이 아닐까 싶습니다.
  3. memset 시, 잘못된 byte 크기가 주어지는 것을 수정했습니다.

@jhpark816
Copy link
Contributor

@namsic 리뷰 바랍니다.

Comment on lines 941 to 944
if (strlen(serverlist) >= ARCUS_MAX_PROXY_FILE_LENGTH) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 형태로 Werror를 회피하는 것보다, snprintf를 사용하는 구현이 나을 것으로 생각합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

현재 do_arcus_zk_update_cachelist_by_string 함수에 주어지는
serverlist의 크기는 9728로 버퍼의 크기인 10240(ARCUS_MAX_PROXY_FILE_LENGTH)보다 작고,
snprintf로 인해 문자열이 잘리게 되어 잘못된 문자열이 저장되는 현상은 없다고 생각되지만
확실하게 보장하는 편이 낫다고 생각하여 이전 PR(strncpy)과 동일하게 수정했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이전 PR에서는 함수가 에러를 반환할 수 있기 때문에 strncpy() 호출 전에 src 길이를 확인하고 에러를 반환하는 것은 나름대로 합리적인 동작으로 보입니다,

그런데 현재 PR에서는 반환값이 없으므로 단순히 수행 중단하도록 변경했습니다.
=> 이 때 문자열 길이가 길면 겉으로 드러나는 문제는 없으면서 의도한대로 동작하지 않을 것 같은데, 이렇게 동작해도 무방한가요?

그리고, 현재 구현은 메모리 누수 가능성도 있어 보입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

메모리 누수 가능성은 확인했습니다.
return 부분에 free를 추가하거나 로직 순서를 변경하는 것이 나을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그런데 현재 PR에서는 반환값이 없으므로 단순히 수행 중단하도록 변경했습니다.
=> 이 때 문자열 길이가 길면 겉으로 드러나는 문제는 없으면서 의도한대로 동작하지 않을 것 같은데, 이렇게 동작해도 무방한가요?

결국은 서버리스트 update를 하지 않게 되므로 무방하지는 않습니다. 하지만 sprintf를 활용한다면 더 위험해지지 않을까 생각됩니다. sprintf의 경우 끝에 \0을 추가하여 컴파일 에러를 보장해주는 것은 맞지만 잘못된 문자열이 만들어질 수 있습니다. 이 상황에서 서버리스트를 update하여 distributio hash를 변경(run_distribution)한다면, 잘못된 동작을 하게 됩니다.

해당 함수는 내부 함수로 사용하는 인자에서 길이가 너무 큰 문자열을 보내는 경우가 없으며, 실제로 서버리스트의 변화는 초기의 arcus_connect 수행에서 arcus_zk_manager 라는 별도의 쓰레드를 두어 감시하고 있는 것으로 보입니다. 그렇기 때문에 별다른 에러 로직을 두지 않고, 바로 리턴을 시킨게 아닐까 싶습니다.

@@ -556,7 +556,7 @@ static inline int do_arcus_cluster_validation_check(memcached_st *mc, arcus_st *
static inline int do_add_client_info(arcus_st *arcus)
{
int result;
char path[250];
char path[512];
Copy link
Collaborator

@namsic namsic Jan 30, 2024

Choose a reason for hiding this comment

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

client_list znode의 최대 길이는 400 이하인 것으로 보이므로, 512 정도면 크게 문제는 없을 것으로 생각합니다.

@@ -30,7 +30,7 @@ struct arcus_zk_st
clientid_t myid;
char ensemble_list[1024];
char svc_code[256];
char path[256];
char path[512];
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 path 버퍼 크기가 512로 변경된 이유는 무엇인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

truncated writing error와 동일하며, 위의 sprintf를 활용하여 문자열 구성할 때 svc_code 변수의 크기와 path 변수 크기와 동일하여 담는 문자열이 path 변수 크기를 초과할 수 있기에 생기는 에러입니다. 에러 내용으로보아 512byte까지는 필요 없다고 생각이 드나 2의 지수로 설정하는 편이 깔끔해 보였고, 넉넉한 공간 확보하는 편이 좋다고 생각이 들어 512byte로 수정했습니다.

libmemcached/arcus.cc:530:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 233 [-Werror=format-truncation=]
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:529:11: note: ‘snprintf’ output between 24 and 279 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc:541:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:540:11: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc: In function ‘arcus_return_t arcus_connect(memcached_st*, const char*, const char*)’:
libmemcached/arcus.cc:131:14: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ^~~~~~~
libmemcached/arcus.cc:130:13: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
     snprintf(arcus->zk.path, sizeof(arcus->zk.path),
     ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc:530:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 233 [-Werror=format-truncation=]
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:529:11: note: ‘snprintf’ output between 24 and 279 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc:541:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:540:11: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc: In function ‘arcus_return_t arcus_pool_connect(memcached_pool_st*, const char*, const char*)’:
libmemcached/arcus.cc:131:14: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ^~~~~~~
libmemcached/arcus.cc:130:13: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
     snprintf(arcus->zk.path, sizeof(arcus->zk.path),
     ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc:530:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 233 [-Werror=format-truncation=]
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:529:11: note: ‘snprintf’ output between 24 and 279 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc:541:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:540:11: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc: In function ‘arcus_return_t arcus_proxy_create(memcached_st*, const char*, const char*)’:
libmemcached/arcus.cc:131:14: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ^~~~~~~
libmemcached/arcus.cc:130:13: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
     snprintf(arcus->zk.path, sizeof(arcus->zk.path),
     ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc:530:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 233 [-Werror=format-truncation=]
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:529:11: note: ‘snprintf’ output between 24 and 279 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_REPL_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc:541:5: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ^~~~~~~
libmemcached/arcus.cc:540:11: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
   snprintf(arcus->zk.path, sizeof(arcus->zk.path),
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmemcached/arcus.cc: In function ‘arcus_return_t arcus_proxy_connect(memcached_st*, memcached_pool_st*, memcached_st*)’:
libmemcached/arcus.cc:131:14: error: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 238 [-Werror=format-truncation=]
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ^~~~~~~
libmemcached/arcus.cc:130:13: note: ‘snprintf’ output between 19 and 274 bytes into a destination of size 256
     snprintf(arcus->zk.path, sizeof(arcus->zk.path),
     ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              "%s/%s", ARCUS_ZK_CACHE_LIST, arcus->zk.svc_code);
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@jhpark816 jhpark816 merged commit 0e64670 into naver:develop Feb 4, 2024
1 check passed
@ing-eoking ing-eoking deleted the zk branch February 4, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants