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

avoid passing incompatible pointer types to message_get_bytes_header #675

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

scareything
Copy link
Member

fixes builds on gcc 14.

@scareything scareything requested a review from a team as a code owner June 27, 2024 13:18
@@ -90,7 +90,7 @@ bool message_get_int32_header(message *m, int header_id, int32_t *v);

bool message_get_uint64_header(message *m, int header_id, uint64_t *v);

bool message_get_bytes_header(message *m, int header_id, const char **ptr, size_t *len);
bool message_get_bytes_header(message *m, int header_id, uint8_t **ptr, size_t *len);
Copy link
Member

Choose a reason for hiding this comment

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

that is a bad idea -- this returns the pointer to data owned by the message -- therefore marked const to the caller.
The caller cannot assume that the data is modifiable.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed signature back to const.

library/bind.c Outdated
@@ -381,7 +381,7 @@ static void process_dial(struct binding_s *b, message *msg) {
bool caller_id_sent = message_get_bytes_header(msg, CallerIdHeader, &source_identity, &source_identity_sz);

ziti_client_ctx clt_ctx = {0};
message_get_bytes_header(msg, AppDataHeader, (uint8_t **) &clt_ctx.app_data, &clt_ctx.app_data_sz);
message_get_bytes_header(msg, AppDataHeader, &clt_ctx.app_data, &clt_ctx.app_data_sz);
Copy link
Member

Choose a reason for hiding this comment

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

it would be safer to make a copy here -- otherwise app_data points to garbage once msg is discarded

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, the variable scoping has not been changed in this PR.

I believe cit_ctx->app_data will be assigned the pointer from the message header. clt_ctx is a stack variable, which is only passed to the client callback. We do document that the client context is only valid while the callback is on the stack.

Copy link
Member

Choose a reason for hiding this comment

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

I think to make it more clear both clt_ctx and ctl_ctx.app_data should be const in the callback

Copy link
Member

@ekoby ekoby left a comment

Choose a reason for hiding this comment

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

current app_data header usage seems unsafe

@scareything scareything added the minor bump minor version label Jun 28, 2024
@scareything scareything merged commit 925e73b into main Jun 28, 2024
9 checks passed
@scareything scareything deleted the fix.gcc14.build branch June 28, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor bump minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants