Skip to content

Fix variables case sensitiveness #1810

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+4 −2
Closed

Fix variables case sensitiveness #1810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
v3.0.3 - YYYY-MMM-DD (to be released)
-------------------------------------

- Fix variables case sensitiveness
[Issue #1808 - @victorhora, @theMiddleBlue, @michaelgranzow-avi]
- Allow empty strings to be evaluated by regex::searchAll
[Issue #1799, #1785 - @victorhora, @XuanHuyDuong, @zimmerle]
- Adds basic pkg-config info
Expand Down
4 changes: 2 additions & 2 deletions src/collection/backend/in_memory-per_process.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#include "src/collection/backend/in_memory-per_process.h"

#ifdef __cplusplus
#include <string>
#include <string.h>
#include <iostream>
#include <unordered_map>
#include <list>
Expand Down Expand Up @@ -106,7 +106,7 @@ void InMemoryPerProcess::resolveMultiMatches(const std::string& var,
}
} else {
for (auto &a : *this) {
if (a.first.compare(0, var.size(), var) == 0) {
if (strncasecmp(a.first.c_str(), var.c_str(), a.first.size()) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring the change from case-sensitive to case-insensitive (which is the correct fix):

The old code compares the first var.size() characters, the new code compares the first a.first.size() characters. If these two sizes are not the same, this is not equivalent.

Consider var = "tx.paranoia", key = "tx.paranoia_level": the old comparison is true, the new one false.

I don't know why the logic was changed from full comparison to partial comparison in the first place (it looks wrong to me), but with this commit we're changing the logic again.

It would be good to learn about the motivation of the original change (892beb5). One aspect is that it turned a hash lookup into a linear search, so it may well have performance impact.

Copy link
Member

@airween airween Jun 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Consider var = "tx.paranoia", key = "tx.paranoia_level": the old comparison is true, the new one false.

Sorry for the question, but why is the old way accepted? In my opinion, any strNcmp is incomprehensible. If I want to set a variable with name tx.paranoia, I want to search it exactly, not the ones which starts with it. :)

I mean, the old any new method is wrong :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@airween : I agree with you, as I said in my comment. For the purpose of this PR, however, I was just pointing out the change in logic that is not related to case sensitivity, which is the only thing this commit should change.

I've raised the question of comparing only the first N bytes on the original PR.

Copy link
Member

Choose a reason for hiding this comment

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

@michaelgranzow-avi : sorry, may be you misunderstood me. I don't understand why is there any strn... comparing, instead of "simple" strcmp()/strcasecmp()? Why just need to compare the first N byte, why not the two strings as they are, without any length argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

@airween, I know what you mean and I agree with you. I've opened #1818 and added a patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Proposed fix: #1820 - this would make the current PR #1810 unnecessary.

l->insert(l->begin(), new VariableValue(&m_name, &var, &a.second));
}
}
Expand Down