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

scap_platform_init and scap_open api leads to memory corruptions #1955

Open
albe19029 opened this issue Jul 11, 2024 · 4 comments
Open

scap_platform_init and scap_open api leads to memory corruptions #1955

albe19029 opened this issue Jul 11, 2024 · 4 comments
Labels
kind/bug Something isn't working

Comments

@albe19029
Copy link
Contributor

albe19029 commented Jul 11, 2024

We have faced memory corruption while switching form version 6.0.1+driver to 7.0.0+driver.
The reason is not very clear API for new scap_platform_init method.

Before this update there was only one method scap_open, with next signature.

scap_t* scap_open(scap_open_args* oargs, char *error, int32_t *rc)

And we passed

char error[SCAP_LASTERR_SIZE] = { 0 };

from the stack without any problems.

scap_platform_init has a bit the same signature:

int32_t scap_platform_init(struct scap_platform *platform, char *lasterr, struct scap_engine_handle engine,
struct scap_open_args *oargs);

But for some reason, internally scap_linux_init_platform method assign lasterr pointer:

linux_platform->m_lasterr = lasterr;

And when we log to this buffer (in our case on stack) - this leads to corruption.

I think that api is not clear. Both method should work the same. For scap_t memory is preallocated in structure.

char m_lasterr[SCAP_LASTERR_SIZE];

Why the code is working in another way for struct scap_linux_platform and don't create this buffer in structure.

@albe19029 albe19029 added the kind/bug Something isn't working label Jul 11, 2024
@albe19029
Copy link
Contributor Author

Or a least document that scap_platform_init has action like this, and buffer must be available till platform will be released.

@FedeDP
Copy link
Contributor

FedeDP commented Jul 26, 2024

Hi! Thanks for opening this issue! Yes the low level libscap library is not really thought to be used in the wild; most of the time, people are using libsinsp instead.
Also pinging @gnosek that is the main author of the platform API :)

@gnosek
Copy link
Contributor

gnosek commented Jul 31, 2024

the low level libscap library is not really thought to be used in the wild

Exactly :) This goes double for scap_platform :)

Or a least document that scap_platform_init has action like this, and buffer must be available till platform will be released.

I think this is the best solution. As long as we have all the moving pieces inside libscap (platform, engine, and the scap_t wrapper itself), we don't want to have distinct error buffers between them (this leads either to copying the error messages back & forth, or to dropping them when we forget to copy them). So, scap_platform shares its buffer with the caller (which is usually scap_init that has its own allocated buffer in scap_t).

Please note that the stack-allocated buffer you're passing to scap_open was used only when scap_open failed and there was no handle to store the error in (we now have allocation and initialization separated and IIRC when scap_init fails, it logs into the handle's buffer).

Both method should work the same.

I respectfully disagree. The two APIs are on two different layers of abstraction (scap_t abstracts over the engine and the platform, and honestly does little useful work).

BTW @albe19029 is your project open source? Can you share a link if so?

@albe19029
Copy link
Contributor Author

Good day @gnosek
Our project is not open source, so I can't share any link with you. Hope it is not a problem as GPL2.txt allows to use it without changes, that is why I usually create request there to make modifications in you repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants