Skip to content

Add "harmful" library#16366

Merged
alexey-milovidov merged 25 commits intomasterfrom
harmful
Jan 4, 2021
Merged

Add "harmful" library#16366
alexey-milovidov merged 25 commits intomasterfrom
harmful

Conversation

@alexey-milovidov
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov commented Oct 26, 2020

Changelog category (leave one):

  • Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add a library that replaces some libc functions to traps that will terminate the process.

Motivated by: https://clickhouse-test-reports.s3.yandex.net/0/c13ab94f91ed4fddd4b9b87adadad68c0ade8684/stress_test_(thread)/stderr.log

Caveats:

  1. The list of harmful functions should be different for main ClickHouse source code and for third-party libraries. For example, it's profane to use fopen strtok, qsort, bsearch, strptime, localtime, gmtime, drand48_r... in the main source code, but they are Ok for third-party libraries.
  2. It will be better to provide different libc headers for the main source code of ClickHouse that will simply lack these functions or mark them as deprecated.
  3. If it's not possible to implement with custom headers, we can also control it on linking stage.
  4. It's mostly unrelated but also worth doing to protect clickhouse-odbc-bridge and ClickHouse itself with seccomp.
  5. I personally would like if dlopen will terminate the program immediately. But we are using it in two places and glibc also using it.

@robot-clickhouse robot-clickhouse added the pr-build Pull request with build/testing/packaging improvement label Oct 26, 2020
@alexey-milovidov
Copy link
Copy Markdown
Member Author

@nikitamikhaylov It works perfectly: SELECT roundBankers(WelchTTest(left, right).2, 16) from welch_ttest;

@alexey-milovidov
Copy link
Copy Markdown
Member Author

+ you have to name aggregate function camelCase, not CamelCase.

@nikitamikhaylov
Copy link
Copy Markdown
Member

+ you have to name aggregate function camelCase, not CamelCase.

I thought their name are case-independent. In any case I specified this property in the code

@alexey-milovidov
Copy link
Copy Markdown
Member Author

No, for functions that are specific for ClickHouse (as opposed to SQL compatible), better to use case-sensitive names.

@alexey-milovidov
Copy link
Copy Markdown
Member Author

#16407

@alexey-milovidov
Copy link
Copy Markdown
Member Author

Should be disabled for clickhouse-odbc-bridge.

@alexey-milovidov
Copy link
Copy Markdown
Member Author

Will enable harmful library only in debug builds.

@qoega
Copy link
Copy Markdown
Member

qoega commented Dec 30, 2020

getservbyname

struct servent * se = getservbyname(port_str.c_str(), nullptr);

@alexey-milovidov
Copy link
Copy Markdown
Member Author

#18647

@alexey-milovidov alexey-milovidov self-assigned this Jan 4, 2021
@alexey-milovidov alexey-milovidov merged commit ca56f83 into master Jan 4, 2021
@alexey-milovidov alexey-milovidov deleted the harmful branch January 4, 2021 22:59
azat added a commit to azat/ClickHouse that referenced this pull request Jan 8, 2021
This will also fix integrity check (ClickHouse#18811), the reason it does not pops
up on CI during PR testing because CI does not set
YANDEX_OFFICIAL_BUILD, while it does set it for commits merged into
upstream commits, and hence once it was merged the special build was
failed.

Follow-up for: ClickHouse#16366
@azat azat mentioned this pull request Jan 8, 2021
@abyss7
Copy link
Copy Markdown
Contributor

abyss7 commented Jan 9, 2021

The log in the description is lost - so what was the motivation?

@alexey-milovidov
Copy link
Copy Markdown
Member Author

@abyss7 Don't allow to use harmful functions in code.

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

Labels

pr-build Pull request with build/testing/packaging improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants