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

ibuf: don't clash with tarantool's ibuf_*() symbols #111

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

Totktonada
Copy link
Member

Copy ibuf.c and ibuf.h from small into the module's code and rename the functions to memcached_ibuf_*().

See tarantool/tarantool#6873 for the root problem and #59 (comment) for analysis.

ibuf_*() symbols may clash with the same named symbols in tarantool, so it worth to rename them to avoid effects of different binary layouts: assertion fails and memory corruptions.

It is the last known problem from #59. All others are confirmed as safe to ignore if we'll keep current libsmall version in the module (see the issue).

More details can be found in the commit messages.

Fixes #59

@Totktonada Totktonada requested a review from ligurio March 28, 2022 23:28
@Totktonada Totktonada force-pushed the Totktonada/gh-59-use-own-ibuf branch from 0d47862 to e8ea5c3 Compare March 28, 2022 23:29
@Totktonada Totktonada added the do not merge Not ready to be merged label Mar 28, 2022
@Totktonada
Copy link
Member Author

I want to proceed with it after PR #102.

@Totktonada Totktonada requested a review from LeonidVas March 28, 2022 23:31
@ligurio ligurio self-requested a review March 29, 2022 11:27
Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

In general, I'm ok.

LGTM

@Totktonada Totktonada force-pushed the Totktonada/gh-59-use-own-ibuf branch from e8ea5c3 to 33912be Compare March 29, 2022 13:15
Just copy and change includes. It is necessary to change function names
in a next commit.

The source is the small version, which is bundled into the module:
1.1-63-g3df5050.

Part of #59
See [1] for the problem root and [2] for analysis. In brief: `ibuf_*()`
symbols may clash with the same named symbols in tarantool, so it worth
to rename them to avoid effects of different binary layouts: assertion
fails and memory corruptions.

For example, `ibuf_reserve_slow()` calls `slab_get()` and `slab_put()`.
If something will be changed in tarantool's slab cache, we can meet
problems in the module.

This patch just renames the `ibuf_*()` functions. It is the most simple
and durable workaround of the problem.

It is likely not necessary to rename inline functions, but since I'm not
dead sure, I renamed them all.

It is the last known problem from #59. All others are confirmed as safe
to ignore if we'll keep current libsmall version in the module (see the
analysis in [2]).

[1]: tarantool/tarantool#6873
[2]: #59 (comment)

Fixes #59
I wrote it mainly to clarify when the hack could be removed.

Follows up #59
@Totktonada Totktonada force-pushed the Totktonada/gh-59-use-own-ibuf branch from 33912be to 50ce479 Compare April 7, 2022 13:55
@Totktonada
Copy link
Member Author

Rebased after PR #102.

@Totktonada Totktonada removed the do not merge Not ready to be merged label Apr 7, 2022
Copy link
Collaborator

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

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

LGTM.

@Totktonada Totktonada merged commit f5ce080 into master Apr 27, 2022
@Totktonada Totktonada deleted the Totktonada/gh-59-use-own-ibuf branch April 27, 2022 15:45
@Totktonada Totktonada mentioned this pull request Dec 6, 2022
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.

Segfault if used with tarantool debug build
3 participants