Skip to content

Commit

Permalink
[#994] Fix SIG-11 when attempting to issue a GVSUBOFLOW error
Browse files Browse the repository at this point in the history
Background
----------
Below is pasted from  https://gitlab.com/YottaDB/DB/YDB/-/issues/994#note_1385299862

* While fuzz testing YDBOcto, I noticed this YDB issue.

* The following query caused a SIG-11 in a Release build of YottaDB and an assert failure in a Debug build
  of YottaDB.

  ```sql
  OCTO> SELECT * from names where id = 1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111;
  ```

* A much simpler test case is provided below that does not rely on Octo and has the same issue.

  ```m
  $ cat gvsuboflow.m
  test	;
  	set $etrap="write $extract($zstatus,1,103),! set $ecode="""""
  	for max=1020:1:1050 do gvsuboflow(max)
  	quit

  gvsuboflow(max)	;
  	set x=$tr($justify(1,max)," ",1) w ^names(x)
  	quit
  ```

  **Release build output**
  ```c
  $ yottadb -run gvsuboflow
  .
  .
  150372986,gvsuboflow+1^x,%YDB-E-GVSUBOFLOW, Maximum combined length of subscripts exceeded,%YDB-I-GVIS,
  %YDB-F-KILLBYSIGSINFO1, YottaDB process 16656 has been killed by a signal 11 at address 0x00007FD1F5253819 (vaddr 0x000055A400313131)
  ```

  **Debug build output**
  ```c
  $ yottadb -run gvsuboflow
  .
  .
  150372986,gvsuboflow+1^x,%YDB-E-GVSUBOFLOW, Maximum combined length of subscripts exceeded,%YDB-I-GVIS,
  %YDB-F-ASSERT, Assert failed in sr_port/format2zwr.c line 47 for expression (ZWR_EXP_RATIO(src_len) <= max_len)
  ```

Issue
-----
* Running the above test case with a Release build of YottaDB, that also is built with ASAN, clearly showed
  where the memory overflow was happening. It was a `stack-buffer-overflow` in `gvsub2str.c`.

  ```
  =================================================================
  ==66729==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fd9e9dee900 at pc 0x7fd9e8bd7175 bp 0x7ffff20985a0 sp 0x7ffff2098590
  WRITE of size 1 at 0x7fd9e9dee900 thread T0
  150372986,gvsuboflow+1^gvsuboflow,%YDB-E-GVSUBOFLOW, Maximum combined length of subscripts exceeded,%YD
      #0 0x7fd9e8bd7174 in gvsub2str sr_port/gvsub2str.c:77
      #1 0x7fd9e8b5cab2 in format_targ_key sr_port/format_targ_key.c:63
      #2 0x7fd9e8d14198 in mval2subsc sr_port/mval2subsc.c:309
      #3 0x7fd9e8d3ca14 in op_gvname_common sr_port/op_gvname.c:133
      #4 0x7fd9e8d3d937 in op_gvname_fast sr_port/op_gvname.c:81
  ```

Fix
---
* The overflow in `gvsub2str.c` is fixed by ensuring that we never go past the buffer bounds by the use
  of a new `ptr_top` local variable. This is done in both cases where `xlat_flag` is TRUE and FALSE.

* Additionally, before calling `format2zwr()` in `gvsub2str.c`, we check if the zwrite expansion of
  the input (`in_length`) would be greater than the output length (`targ_len`). If so, we reduce `in_length`
  by enough so its zwrite expansion would not exceed `targ_len`. We use a new `ZWR_EXP_RATIO_INVERT` macro
  to get this value (it is the converse of the ZWR_EXP_RATIO macro). This is necessary to avoid failing
  a pre-existing assert in `format2zwr.c` that verifies all callers have allocated enough space in the
  output for the given input. In this case in `gvsub2str.c`, we cannot increase `targ_len` and so we
  truncate `in_length` enough so they are accepted by `format2zwr()`. This means we would see a truncated
  output but that is okay considering this usually would be part of a GVSUBOFLOW error which would show
  a huge subscript. Seeing the first few subscripts should be enough in that case for the user to figure
  out the problem and correct it. No need to see the entire input subscripts.
  • Loading branch information
nars1 committed Jul 28, 2023
1 parent 14e071d commit cb4697b
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
23 changes: 21 additions & 2 deletions sr_port/gvsub2str.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* Copyright (c) 2001-2021 Fidelity National Information *
* Services, Inc. and/or its subsidiaries. All rights reserved. *
* *
* Copyright (c) 2023 YottaDB LLC and/or its subsidiaries. *
* All rights reserved. *
* *
* This source code contains the intellectual property *
* of its copyright holder(s), and is made available *
* under a license. If you do not know the terms of *
Expand Down Expand Up @@ -67,9 +70,19 @@ unsigned char *gvsub2str(unsigned char *sub, mstr *opstr, boolean_t xlat_flg)
targ = (unsigned char *)opstr->addr;
if (STR_SUB_PREFIX == ch || (SUBSCRIPT_STDCOL_NULL == ch && KEY_DELIMITER == *sub))
{ /* If this is a string */
unsigned char *ptr_top;

in_length = 0;
ptr = (xlat_flg ? buf : targ);
while ((ch = *sub++))
if (xlat_flg)
{
ptr = buf;
ptr_top = ptr + SIZEOF(buf);
} else
{
ptr = targ;
ptr_top = ptr + opstr->len;
}
while ((ch = *sub++) && (ptr < ptr_top))
{ /* Copy string to ptr, xlating each char */
in_length++;
if (STR_SUB_ESCAPE == ch) /* if this is an escape, demote next char */
Expand Down Expand Up @@ -111,6 +124,12 @@ unsigned char *gvsub2str(unsigned char *sub, mstr *opstr, boolean_t xlat_flg)
if (xlat_flg)
{
targ_len = opstr->len;
if (ZWR_EXP_RATIO(in_length) > targ_len)
{ /* Ensure zwrite expansion of "in_length" will not exceed "targ_len".
* Or else "format2zwr()" will fail a similar assert.
*/
in_length = ZWR_EXP_RATIO_INVERT(targ_len);
}
format2zwr((sm_uc_ptr_t)ptr, in_length, targ, &targ_len);
assert(targ_len <= opstr->len);
targ = targ + targ_len;
Expand Down
2 changes: 2 additions & 0 deletions sr_port/mdef.h
Original file line number Diff line number Diff line change
Expand Up @@ -544,9 +544,11 @@ MBSTART { \
*/
#ifdef UTF8_SUPPORTED
# define ZWR_EXP_RATIO(X) (((X) * 9 + 11))
# define ZWR_EXP_RATIO_INVERT(X) (((X) - 11) / 9)
# define MAX_ZWR_DCHAR_DIGITS 5
#else
# define ZWR_EXP_RATIO(X) ((X) * 6 + 7)
# define ZWR_EXP_RATIO_INVERT(X) (((X) - 7) / 6)
# define MAX_ZWR_DCHAR_DIGITS 3
#endif
#define MAX_ZWR_KEY_SZ ZWR_EXP_RATIO(MAX_KEY_SZ)
Expand Down

0 comments on commit cb4697b

Please sign in to comment.