Skip to content
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

Jump to bottom

SIG-6 (SIGABRT) fatal error and core from MUPIP BACKUP when backup filename path length is nearly 255 characters #267

Closed
nars1 opened this issue May 31, 2018 · 0 comments
Closed

SIG-6 (SIGABRT) fatal error and core from MUPIP BACKUP when backup filename path length is nearly 255 characters #267

nars1 opened this issue May 31, 2018 · 0 comments
Assignees
@nars1
Labels
upstream issue
Milestone
r124

Comments

@nars1
Copy link
Collaborator

nars1 commented May 31, 2018

Final Release Note

Description

Below is a backup invocation that demonstrates the issue.

$ $ydb_dist/mupip BACKUP DEFAULT /extra1/testarea1/user/V998/tst_V998_R122_dbg_01_180531_094328/v63003_0/gtm4212/aaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaa

*** buffer overflow detected ***: /usr/library/V999_R122/pro/mupip terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7ffad2c7a7e5]
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7ffad2d1c15c]
/lib/x86_64-linux-gnu/libc.so.6(+0x117160)[0x7ffad2d1a160]
/usr/library/V999_R122/pro/libyottadb.so(+0x240073)[0x7ffad276a073]
/usr/library/V999_R122/pro/libyottadb.so(+0x137583)[0x7ffad2661583]
/usr/library/V999_R122/pro/libyottadb.so(mupip_main+0x12d)[0x7ffad255384d]
/usr/library/V999_R122/pro/mupip(dlopen_libyottadb+0x1dc)[0x400d0c]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7ffad2c23830]
/usr/library/V999_R122/pro/mupip(_start+0x29)[0x400a59]
======= Memory map: ========
00400000-00402000 r-xp 00000000 fd:00 1766629                            /usr/library/V999_R122/pro/mupip
00601000-00602000 r--p 00001000 fd:00 1766629                            /usr/library/V999_R122/pro/mupip
00602000-00603000 rw-p 00002000 fd:00 1766629                            /usr/library/V999_R122/pro/mupip
01aff000-01c2f000 rw-p 00000000 00:00 0                                  [heap]
.
.
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
%YDB-F-KILLBYSIGSINFO1, MUPIP process 7377 has been killed by a signal 6 at address 0x00007FFAD2C38428 (vaddr 0x000000E400001CD1)

Draft Release Note

MUPIP BACKUP correctly issues a FILENAMETOOLONG error when the absolute path length of the target backup filename is too long (approximately 255 bytes). Previously, this used to result in an abnormal termination of the backup process with a KILLBYSIGSINFO1 fatal error (signal 6/SIGABRT) due to a buffer overflow.
@nars1 nars1 added this to the r124 milestone May 31, 2018
@nars1 nars1 self-assigned this May 31, 2018
nars1 added a commit to nars1/YottaDB that referenced this issue May 31, 2018
@nars1
[YottaDB#267] Fix a few issues identified by test failures
453222d 
1) / is not guaranteed to be last byte in tempdir_full.addr in various cases.
   So the / that was removed from the prior YottaDB#267 commit is inserted back.

2) Previously, tempdir_full.addr held the fully constructed name but it no longer does
   after the prior YottaDB#267 commit. So tempdir_full.addr usages after tempfilename has been
   constructed need to be replaced with tempfilename.

3) One use of orig_buff_size was replaced with SIZEOF(tempfilename) since the latter
   is what got used as the maximum size in the SNPRINTF call. Both the values hold the
   same value so there is no issue but still it is better/safer to be consistent.
nars1 added a commit that referenced this issue May 31, 2018
@nars1
[#267] Fix a few issues identified by test failures
bbae242 
1) / is not guaranteed to be last byte in tempdir_full.addr in various cases.
   So the / that was removed from the prior #267 commit is inserted back.

2) Previously, tempdir_full.addr held the fully constructed name but it no longer does
   after the prior #267 commit. So tempdir_full.addr usages after tempfilename has been
   constructed need to be replaced with tempfilename.

3) One use of orig_buff_size was replaced with SIZEOF(tempfilename) since the latter
   is what got used as the maximum size in the SNPRINTF call. Both the values hold the
   same value so there is no issue but still it is better/safer to be consistent.
nars1 added a commit that referenced this issue May 31, 2018
@nars1
[#264] Fix a few issues identified by test failures
5015093 
1) / is not guaranteed to be last byte in tempdir_full.addr in various cases.
   So the / that was removed from the prior #267 commit is inserted back.

2) Previously, tempdir_full.addr held the fully constructed name but it no longer does
   after the prior #267 commit. So tempdir_full.addr usages after tempfilename has been
   constructed need to be replaced with tempfilename.

3) One use of orig_buff_size was replaced with SIZEOF(tempfilename) since the latter
   is what got used as the maximum size in the SNPRINTF call. Both the values hold the
   same value so there is no issue but still it is better/safer to be consistent.
nars1 added a commit that referenced this issue Jun 1, 2018
@nars1
[#267] Fix buffer overflow in MUPIP BACKUP that can cause SIGABRT/SIG…
83bf0ad 
…-6 errors

The check for whether the temporary file name is longer than the allocated buffer can hold
was incorrect. It was assuming 4 bytes (SIZEOF(uint4)) for the string that followed the
hexadecimal process-id but the actual name had _XXXXXX following it which accounts for 7
bytes. For example, the actual temporary file name was DEFAULT_17fa_XXXXXX (where XXXXXX
was replaced with a temporary string by "mkstemp") but the check was accounting only
for DEFAULT_17fa_XXX. So there was a potential for a 3-byte overflow to go unnoticed.
This logic is now reworked to use SNPRINTF to let us accurately know if the allocated
buffer is not enough and if so issue a FILENAMETOOLONG error.

In addition, the FILENAMETOOLONG error is now prefixed with a FILEPARSE error message
which prints the entire path name that exceeded the 255 byte limit. Previously this
only printed the absolute directory path without including the temporary file name. This
could be potentially confusing to the user since they would see the directory path printed
in the FILEPARSE error message is less than 255 characters but still an error is being
issued saying it is longer than 255 characters.

This is the 2nd commit message:

[#267] Fix a few issues identified by test failures

1) / is not guaranteed to be last byte in tempdir_full.addr in various cases.
   So the / that was removed from the prior #267 commit is inserted back.

2) Previously, tempdir_full.addr held the fully constructed name but it no longer does
   after the prior #267 commit. So tempdir_full.addr usages after tempfilename has been
   constructed need to be replaced with tempfilename.

3) One use of orig_buff_size was replaced with SIZEOF(tempfilename) since the latter
   is what got used as the maximum size in the SNPRINTF call. Both the values hold the
   same value so there is no issue but still it is better/safer to be consistent.

This is the 3rd commit message:

[#267] Fix more test failures

* Zero pad the hexadecimal pid in the backup temporary file name to keep its length
  at 4-bytes even if the process_id is less than 2**24

* 4-byte pid translates to 8 hexadecimal digits so need 8x instead of 4x in the SNPRINTF
  format string.

* Remove duplication of the format string by moving it to a macro
  TEMP_FILE_FMT_STRING. This fixes a few differences in the format string used in the
  two places.
This was referenced Jun 1, 2018
Merged
Merged
@nars1 nars1 closed this as completed Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nars1 nars1

Labels
upstream issue
Projects
None yet
Milestone
r124
Development

No branches or pull requests
1 participant
@nars1