You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by xiaozhongwang <gi...@git.apache.org> on 2018/01/12 04:27:50 UTC
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
GitHub user xiaozhongwang opened a pull request:
https://github.com/apache/trafodion/pull/1394
[TRAFODION-2891] fix the bufoverrun Critical error checked by TScanCode
fix the bufoverrun Critical error checked by TScanCode
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/xiaozhongwang/trafodion TRAFODION-2891
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1394.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1394
----
commit 0166c96321a104636e7dec67eb98b67e931ea84e
Author: Kenny <xi...@...>
Date: 2018-01-11T08:32:39Z
fix the bufoverrun Critical error checked by TScanCode
commit d6573f9c0a1ca7a7f63f76a629e64750a37f038c
Author: Kenny <xi...@...>
Date: 2018-01-12T01:37:08Z
fix the bufoverrun Critical error checked by TScanCode
commit 1fe8890705310df729c35401d53d55531e8e8398
Author: Kenny <xi...@...>
Date: 2018-01-12T04:02:26Z
fix the bufoverrun Critical error checked by TScanCode
----
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1394#discussion_r161806495
--- Diff: core/conn/odbc/src/odbc/nsksrvrcore/srvrothers.cpp ---
@@ -5153,7 +5153,7 @@ odbc_SQLSvc_GetSQLCatalogs_sme_(
{
ERROR_DESC_def *p_buffer = QryCatalogSrvrStmt->sqlError.errorList._buffer;
strncpy(RequestError, p_buffer->errorText,sizeof(RequestError) -1);
- RequestError[sizeof(RequestError)] = '\0';
+ RequestError[sizeof(RequestError) - 1] = '\0';
--- End diff --
very good catch!
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1394#discussion_r161806194
--- Diff: core/conn/odbc/src/odbc/Common/linux/sqmem.cpp ---
@@ -400,7 +400,7 @@ void TestPool(void * membase, int length)
long j, index;
short error;
long pass;
- void * pool_ptrs[256];
+ void * pool_ptrs[256 + 1];
--- End diff --
very good catch!
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1394#discussion_r161805913
--- Diff: core/conn/odb/src/odb.c ---
@@ -5313,7 +5313,7 @@ static void etabadd(char type, char *run, int id)
}
}
if ( etab[no].type == 'e' ) { /* name & create output file */
- for ( i = j = 0; etab[no].tgt[i] && i < sizeof(buff); i++ ) {
+ for ( i = j = 0; i < sizeof(buff) && etab[no].tgt[i]; i++ ) {
--- End diff --
I don't really get this fully understand.
If i is out of bound for arry tgt, why this change will prevent the tag[i] to be executed?
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1394
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by xiaozhongwang <gi...@git.apache.org>.
Github user xiaozhongwang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1394#discussion_r161910390
--- Diff: core/conn/odb/src/odb.c ---
@@ -5313,7 +5313,7 @@ static void etabadd(char type, char *run, int id)
}
}
if ( etab[no].type == 'e' ) { /* name & create output file */
- for ( i = j = 0; etab[no].tgt[i] && i < sizeof(buff); i++ ) {
+ for ( i = j = 0; i < sizeof(buff) && etab[no].tgt[i]; i++ ) {
--- End diff --
Most of time, they are equivalent. There are exception in a special case
Most of compiler judge the condition from left to right, so if the tgt is at the end of memory, and the offset i is equal to sizeof(buff), the access to memory goes beyond memory.
This will make a coredump, and cann't be repeatable.
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1394#discussion_r162226893
--- Diff: core/conn/odb/src/odb.c ---
@@ -5313,7 +5313,7 @@ static void etabadd(char type, char *run, int id)
}
}
if ( etab[no].type == 'e' ) { /* name & create output file */
- for ( i = j = 0; etab[no].tgt[i] && i < sizeof(buff); i++ ) {
+ for ( i = j = 0; i < sizeof(buff) && etab[no].tgt[i]; i++ ) {
--- End diff --
I tried to make sense of this too. It looks like etab[no].tgt is populated in function parseopt from some string buffer; I can't tell if it is buff (which is a static variable declared as a char[256]) or something else. My suspicion is that the test "i < sizeof(buff)" has nothing to do with etab[no].tgt but rather some other buffer being copied to. In any case, reversing the order of these tests seems harmless.
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by xiaozhongwang <gi...@git.apache.org>.
Github user xiaozhongwang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1394#discussion_r161911063
--- Diff: core/sqf/src/seatrans/tm/hbasetmlib2/hbasetm.h ---
@@ -139,9 +139,9 @@ class CHbaseTM : public JavaObjectInterfaceTM
JM_REGTRUNCABORT,
JM_DROPTABLE,
JM_RQREGINFO,
- JM_LAST
+ JM_TMLAST
};
- JavaMethodInit JavaMethods_[JM_LAST];
+ JavaMethodInit JavaMethods_[JM_TMLAST];
--- End diff --
There is another define of JM_LAST in this file, and their value is different.
---
[GitHub] trafodion pull request #1394: [TRAFODION-2891] fix the bufoverrun Critical e...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1394#discussion_r161806802
--- Diff: core/sqf/src/seatrans/tm/hbasetmlib2/hbasetm.h ---
@@ -139,9 +139,9 @@ class CHbaseTM : public JavaObjectInterfaceTM
JM_REGTRUNCABORT,
JM_DROPTABLE,
JM_RQREGINFO,
- JM_LAST
+ JM_TMLAST
};
- JavaMethodInit JavaMethods_[JM_LAST];
+ JavaMethodInit JavaMethods_[JM_TMLAST];
--- End diff --
This is critical!
---