Issue #197: fixed buffer overflow in `_makeWchar()` jv
authorJan Vrany <jan.vrany@fit.cvut.cz>
Wed, 28 Mar 2018 09:51:58 +0100
branchjv
changeset 23101 cd0581d5639b
parent 23100 4e9bbc6019e0
child 23102 574962856f04
Issue #197: fixed buffer overflow in `_makeWchar()` ...when passed string is `Unicode16String`. It (wrongly) used passed buffer size as size of the buffer in character while caller passed it as a size in bytes (a result of `sizeof()`). This naturally caused a buffer overflow when size of (unicode) string was greater (MAXPATHLEN / 2), causing the segmentation violation. This commit fixes this mismatch and makes sure that once `_makeWchar()` returns, it's properly NULL terminated. https://swing.fit.cvut.cz/projects/stx-jv/ticket/197
Win32OperatingSystem.st
--- a/Win32OperatingSystem.st	Mon Mar 05 10:46:56 2018 +0000
+++ b/Win32OperatingSystem.st	Wed Mar 28 09:51:58 2018 +0100
@@ -586,28 +586,45 @@
 
 !Win32OperatingSystem primitiveFunctions!
 %{
-int
-_makeWchar(OBJ string, wchar_t *buffer, int bufferSize)
+
+/**
+ * The `_makeWchar()` function copies contents of a string object `srcObj` to a 
+ * (wide char) buffer `dst`. At most `n` **bytes** including null terminator 
+ * are copied to destination buffer `dst`. 
+ * 
+ * Returns the number of **bytes** copied (including null terminator). If 
+ * source string object is not an instance of `String` or `Unicode16String`, 
+ * returns `-1`.
+ */ 
+static int
+_makeWchar(OBJ srcObj, wchar_t *dst, int n/* in bytes!!! */)
 {
-    int i, len;
-
-    if (__isStringLike(string)) {
-	len = __stringSize(string);
-	if (len > bufferSize) len = bufferSize;
-	for (i=0; i<len; i++) {
-	    buffer[i] = __stringVal(string)[i];
-	}
-    } else if (__isUnicode16String(string)) {
-	len = __unicode16StringSize(string);
-	if (len > bufferSize) len = bufferSize;
-	for (i=0; i<len; i++) {
-	    buffer[i] = __unicode16StringVal(string)[i];
-	}
+    int i;
+    int srcLen; /* length of string to be copied, in **characters** */
+    int dstLen; /* length of destination buffer,  in **characters** */
+
+    dstLen = n / sizeof(wchar_t);
+	
+    if (__isStringLike(srcObj)) {
+	srcLen = __stringSize(srcObj);	
+	
+	if (srcLen >= dstLen) srcLen = dstLen - 1;
+
+	for (i=0; i<srcLen; i++) {
+	    dst[i] = __stringVal(srcObj)[i];
+	}
+    } else if (__isUnicode16String(srcObj)) {    	
+	srcLen = __unicode16StringSize(srcObj);
+	
+	if (srcLen >= dstLen) srcLen = dstLen - 1;		
+	
+	memcpy(dst, __unicode16StringVal(srcObj), srcLen * sizeof(wchar_t));
     } else {
+    	dst[0] = 0;
 	return(-1);
     }
-    buffer[len] = 0;
-    return(len);
+    dst[srcLen] = 0;    
+    return(srcLen * sizeof(wchar_t));
 }
 
 
@@ -6700,7 +6717,7 @@
     if (__isSmallInteger(anInteger)) {
 	if (__isStringLike(aPathName)) {
 #ifdef DO_WRAP_CALLS
-	    char _aPathName[MAXPATHLEN];
+	    char _aPathName[MAXPATHLEN+1];
 
 	    strncpy(_aPathName, __stringVal(aPathName), MAXPATHLEN-1); _aPathName[MAXPATHLEN-1] = '\0';
 	    do {