Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[#724] Fix edge cases with $ZATRANSFORM() and make function more robust
When reviewing op_zatransform.c to add testing for new functionality added in the recent merge of the upstream V63010 version into YottaDB, we noted some coding issues that could cause incorrect operation currently as well as be a source for future problems in the code. The edge cases are as follows: 1. On line 132 of op_fnzatransform.c, a test is made of (0 == msrc->str.len) before ascertaining that the mval has MV_STR set in msrc->mvtype. This causes two invocations of $ZATRANSFORM() with such as 'WRITE $ZATRANSFORM(123,0)' and 'SET X=1 WRITE $ZATRANSFORM(123*x,0)' to display different output values which should not be possible. This test is changed to also verify that the input value *IS* a string before checking its length. 2. On lines 157-163 of op_fnzatransform.c, we are setting TREF(transform) based on the value of the 4th input parameter (forceStr) that denotes whether the input is a string or not. But if one passes in a string but doesn't set the forceStr to 1 (leaving it to default to 0), this turns off transformation which doesn't follow logically. Since no tying of forcing the input to be string to whether or not transformation should be done is documented anywhere, we have removed this tying in favor of assuming we will always be doing transformation and asserting that is true. For the robustness aspect, as far as we could ascertain without a deep-dive review of associated calls, there is nothing dangerously exposed at this time but if code were added in the future without knowledge of the potential problems this code has, results would not be good and could be very tricky to track down. Specifically, this code, early on, makes a copy of the input mval which comes direct from generated M code. This is a coding no-no because if ANY stringpool allocation happens that could potentially trigger a garbage collection, the garbage collector does not know about the copied mval so cannot update any stringpool pointer it has if the data gets moved as a result of the garbage collection. The result is a local mval copy can end up pointing to garbage. Right now, there are no references to the copied variable after actions that can cause garbage collections (of which there are several in this routine) so the design is (probably) safe though that has not been conclusively proven. Out of an abundance of caution, we are going to make this safe now and for the future. To make this module safe for future expansion is fairly simple. One simply needs to use the PUSH_MV_STENT() macro with the MVST_MVAL type which places an mv_stent block on the M stack giving one an embedded mval that is known to the garbage collector. Then we just point the "src" variable at this mval, then place a POP_MV_STENT() at the end of the routine and we are done. We don't have to worry about error returns because an error always pops the current M stack frame which would also pop this mv_stent off of the M stack.
- Loading branch information
