[MDEV-26637] ASAN: main.metadata and user_variables.basic MTR failures after MDEV-26572 Created: 2021-09-18 Updated: 2021-10-27 Resolved: 2021-10-12 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | OTHER |
| Affects Version/s: | 10.7 |
| Fix Version/s: | 10.7.1 |
| Type: | Bug | Priority: | Blocker |
| Reporter: | Alexander Barkov | Assignee: | Oleksandr Byelkin |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||
| Description |
|
After
This is a minimum reproducible script:
The problem was not really introduced by
This quick patch fixes the problem:
This patch assumes that the length "0" is passed only on binary data, while in case of text string data a correct length value is passed. All MTR tests passed with the patch. So this assumption seems to be true. But the patch slows down a very hot function by adding a new if testing hash->charset . It should not be pushed AS IS. It's only a demo showing where the real problem is. The real patch should fix all my_hash_search() calls not to pass 0 as length. |
| Comments |
| Comment by Oleksandr Byelkin [ 2021-09-23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
It would be nice to figure out who need and why we allow user variables with an empty name (zero length name) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Alexander Barkov [ 2021-09-23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think fixing the code not to pass 0 meaning "use length of the other side from hash" would be a more reliable solution. That would fix the source of the problem. It's absolutely for sure that hash abuses the function strnncoll(), which is not supposed to be called this way. strnncoll() requires precise length. On the contrary, disallowing user variables with empty names would fix only one of the visible consequence. We cannot know how many other invisible consequences we have. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Oleksandr Byelkin [ 2021-09-30 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
It is patches as result of the bug investigation. I think they should be moved to 10.2 Only "hash" fix is obligatory fix, all other is fixing "strange things" found.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Elena Stepanova [ 2021-10-03 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Lots of tests fail with a similar stack trace on 10.7 valgrind or MSAN builds. I hope it will be fixed by the same patches.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Michael Widenius [ 2021-10-07 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hi! Here follows the review of each commit: 39a90aad13e30e4c4a7d6046583a1b48a3605a2c ok 2544edd01b41857ff16120ecd5f1ce7cec476d7c As there was no test case, under what conditions could this happen? a3351a6248ff37e42c00f054c76f7349449cc58c ok 4a3834e249769778ed81eab2bb97b7792fa4a039 Would be even better if role would be changed to a LEX_CSTRING, but that f64531a17b5b0828d04e226fd47ed5f7f36840fc This one I don't like. Instead of fixing hashcmp, which can be called many times in a loop, diff --git a/mysys/hash.c b/mysys/hash.c
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Michael Widenius [ 2021-10-07 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Review done. Can be found in comment section | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Roel Van de Paar [ 2021-10-09 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Leads to:
Setup:
Bug confirmed present in: Bug (or feature/syntax) confirmed not present in: |