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

Fix condition in find_node_context_host #269

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Conversation

jajik
Copy link
Collaborator

@jajik jajik commented Aug 16, 2024

I believe the current condition is incorrect, because in case of balancer-s->name is shorter than the prefix, it gets included for node->mess.balancer even though they don't match.

In combination with #267 it prevents 404 when the node is up but without app (yet).

Copy link
Member

@jfclere jfclere left a comment

Choose a reason for hiding this comment

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

Should we have
if (strlen(balancer->s->name) <= BALANCER_PREFIX_LENGTH) {
continue;
}
if (strcasecmp(&balancer->s->name[BALANCER_PREFIX_LENGTH], node->mess.balancer) != 0) {
continue;
}
The test for strlen() is to check that the strcasecmp is not comparing somewhere outside the balancer->s->name... Is that even possible?

@jajik
Copy link
Collaborator Author

jajik commented Aug 21, 2024

Should we have if (strlen(balancer->s->name) <= BALANCER_PREFIX_LENGTH) { continue; } if (strcasecmp(&balancer->s->name[BALANCER_PREFIX_LENGTH], node->mess.balancer) != 0) { continue; } The test for strlen() is to check that the strcasecmp is not comparing somewhere outside the balancer->s->name... Is that even possible?

The proposed change is equivalent to the two ifs you wrote if I read that correctly.

For the second part, I don't think it's possible because BALANCER_PREFIX_LENGTH is smaller than the array size. However, I thought that the strlen should prevent reading invalid balancer names (e.g. if we have a valid balancer name for which we change the first char to \0, we would catch it with the strlen but wouldn't without it).

@jajik jajik force-pushed the fix-condition branch 2 times, most recently from 400af53 to f95e024 Compare August 21, 2024 15:22
@jajik jajik merged commit ce901d8 into modcluster:main Aug 28, 2024
9 checks passed
@jajik jajik deleted the fix-condition branch August 28, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants