sql: fix NULL dereference in where.c #8259
Conversation
There was a ambitious pointer deref in where_loop_builder_shortcut(), that wasn't visible due to dead code. Remove the dead code and add assert before dereference. NO_TEST=refactoring NO_DOC=refactoring NO_CHANGELOG=refactoring
Lord-KA
force-pushed
the
Lord-KA/gh-50-deref-after-null
branch
from
93310fe
to
38394d7
Compare
There was a problem hiding this comment.
Hi! Thank you for the patch! I have a few comments below. Also, I think the current commit title is not appropriate as fix assumes the bug will be fixed, which is definitely not a refactoring. Also, I think the code became dead due to an error, so it might be better to restore it instead of removing.
| assert(space->def != NULL); | ||
| for (uint32_t i = 0; i < space->index_count; ++i) { | ||
| struct index_def *idx_def = | ||
| space->index[i]->def; |
There was a problem hiding this comment.
Why do you need a new line here?
| continue; | ||
| if (where_loop_assign_terms(loop, clause, | ||
| cursor, space->def, | ||
| idx_def) == 0) |
There was a problem hiding this comment.
I think you can use two lines instead of three.
| } | ||
| } else { | ||
| /* Space is ephemeral. */ | ||
| assert(space_def->id == 0); |
There was a problem hiding this comment.
This line looks rather strange. I looked at earlier commits and found that space was first obtained using space_cache_find() which would return NULL for ephemeral spaces. This was later changed to the current version, however the new version does not properly check space for being ephemeral. So most likely the code is not working as intended. I think a more appropriate solution would be to check if the space->def->id is 0 instead of a space! = NULL. However, all the mentioned changes happened 5 years ago, so it is entirely possible that the original design will not be applicable. It should probably be investigated.
There was a ambitious pointer deref in where_loop_builder_shortcut(), that wasn't visible due to dead code.
Removed the dead code and added assert before dereference.
NO_TEST=refactoring
NO_DOC=refactoring
NO_CHANGELOG=refactoring