Stop using unsafe strcpy method

[GoogleCodeExporter]

What steps will reproduce the problem?
$ grep -l -r strcpy *

What is the expected output? What do you see instead?

Expected: No uses of strcpy.

Actual:
src/compiler/ShaderLang.cpp
src/compiler/preprocessor/atom.c
src/compiler/preprocessor/cpp.c
src/compiler/preprocessor/scanner.c
src/compiler/preprocessor/tokens.c
src/libGLESv2/Program.cpp

What version of the product are you using? On what operating system?
svn r1007 on Mac OS X 10.7.3

Please provide any additional information below.

Consider switching to strlcpy, or strncpy with a terminating '\0' write.

Original issue reported on code.google.com by ddkilzer@gmail.com on 22 Mar 2012 at 3:25

[GoogleCodeExporter]

Several months ago I did a quick audit of .*cpy, .*cat and .*printf functions 
in ANGLE, and there were a few questionable cases but nothing I could pin down 
as being specifically problematic.

Nonetheless, I support generically moving to safer functions because code 
changes, I may have missed something, and already I've seen large amounts of 
ANGLE code getting pulled into other projects which magnifies the risk.

I'm curious about this coming in wrt to OS X though. Is ANGLE being used on 
non-Windows?

If it's Windows-only then I'd actually suggest moving to the Microsoft safe 
function variants (strcpy_s and family).

Original comment by ke...@chromium.org on 22 Mar 2012 at 1:40

[GoogleCodeExporter]

Correct, I'm not saying there is any real security issue, just that it's better 
to use the safe variant of strcpy (hardening if you will).

ANGLE has been integrated into the WebCore sub-project of WebKit.  WebKit 
builds on Mac OS X, Windows, Linux, and a variety of other platforms, so we 
should use strcpy_s for Windows and strlcpy for UNIX variants; something like 
this:

diff --git a/src/compiler/ShaderLang.cpp b/src/compiler/ShaderLang.cpp
index 13f11b5..1565abe 100644
--- a/src/compiler/ShaderLang.cpp
+++ b/src/compiler/ShaderLang.cpp
@@ -239,7 +239,12 @@ void ShGetInfoLog(const ShHandle handle, char* infoLog)
     if (!compiler) return;

     TInfoSink& infoSink = compiler->getInfoSink();
-    strcpy(infoLog, infoSink.info.c_str());
+    const size_t len = ShGetInfo(SH_INFO_LOG_LENGTH);
+#if defined(_MSC_VER)
+    strcpy_s(infoLog, len, infoSink.info.c_str());
+#else
+    strlcpy(infoLog, infoSink.info.c_str(), len);
+#endif
 }

 //
@@ -255,7 +260,12 @@ void ShGetObjectCode(const ShHandle handle, char* objCode)
     if (!compiler) return;

     TInfoSink& infoSink = compiler->getInfoSink();
-    strcpy(objCode, infoSink.obj.c_str());
+    const size_t len = ShGetInfo(SH_OBJECT_CODE_LENGTH);
+#if defined(_MSC_VER)
+    strcpy_s(objCode, len, infoSink.obj.c_str());
+#else
+    strlcpy(objCode, infoSink.obj.c_str(), len);
+#endif
 }

 void ShGetActiveAttrib(const ShHandle handle,

Original comment by ddkilzer@gmail.com on 22 Mar 2012 at 3:12

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I'm going to try to work on a patch for this issue.

Original comment by ddkilzer@gmail.com on 26 Apr 2012 at 4:56

[GoogleCodeExporter]

A patch would definitely be appreciated.  However, I'm much prefer if you wrote 
one helper function which contained the #ifdef in it, and call that wrapper 
function from everywhere instead of #ifdef-ing every location where strcpy is 
used.

Original comment by dan...@transgaming.com on 11 May 2012 at 12:53

[GoogleCodeExporter]

Sounds good.  Is there a preferred location for the helper function to reside?

Original comment by ddkilzer@gmail.com on 11 May 2012 at 2:44

[GoogleCodeExporter]

common/angleutils.h would be my suggestion.  If more than a header-only 
implementation is required, we should probably add commom/angleutils.cpp.

Original comment by dan...@transgaming.com on 11 May 2012 at 3:01

[GoogleCodeExporter]

Tracked for WebKit by:  https://bugs.webkit.org/show_bug.cgi?id=129237

Original comment by ddkilzer@gmail.com on 23 Feb 2014 at 11:56

[GoogleCodeExporter]

Fixed in WebKit:  <http://trac.webkit.org/changeset/164580>

Original comment by ddkilzer@gmail.com on 24 Feb 2014 at 11:11

Attachments:

• 0001-ANGLE-Stop-using-unsafe-strcpy-method.patch

[GoogleCodeExporter]

WebKit-contributed patches will make it into ANGLE much more quickly if 
submitted using the process outlined on the wiki: 
https://code.google.com/p/angleproject/wiki/ContributingCode

Original comment by shannonw...@chromium.org on 24 Feb 2014 at 4:07

[GoogleCodeExporter]

Issue 636 has been merged into this issue.

Original comment by jmad...@chromium.org on 5 May 2014 at 2:17

[GoogleCodeExporter]

Did the strcpy fixes ever make it into ANGLE? From issue 636 from achristensen07

diff --git a/src/compiler/translator/ShaderLang.cpp 
b/src/compiler/translator/ShaderLang.cpp
index b98c371..dd26873 100644
--- a/src/compiler/translator/ShaderLang.cpp
+++ b/src/compiler/translator/ShaderLang.cpp
@@ -242,8 +242,12 @@ void ShGetInfoLog(const ShHandle handle, char* infoLog)
     TCompiler* compiler = base->getAsCompiler();
     if (!compiler) return;

+    size_t infoLogLength = 0;
+    ShGetInfo(compiler, SH_INFO_LOG_LENGTH, &infoLogLength);
+
     TInfoSink& infoSink = compiler->getInfoSink();
-    strcpy(infoLog, infoSink.info.c_str());
+    strncpy(infoLog, infoSink.info.c_str(), infoLogLength);
+    infoLog[infoLogLength - 1] = '\0';
 }

 //
@@ -258,8 +262,12 @@ void ShGetObjectCode(const ShHandle handle, char* objCode)
     TCompiler* compiler = base->getAsCompiler();
     if (!compiler) return;

+    size_t objCodeLength = 0;
+    ShGetInfo(handle, SH_OBJECT_CODE_LENGTH, &objCodeLength);
+
     TInfoSink& infoSink = compiler->getInfoSink();
-    strcpy(objCode, infoSink.obj.c_str());
+    strncpy(objCode, infoSink.obj.c_str(), objCodeLength);
+    objCode[objCodeLength - 1] = '\0';
 }

 void ShGetVariableInfo(const ShHandle handle,
diff --git a/src/libGLESv2/Program.cpp b/src/libGLESv2/Program.cpp
index 8a9fb04..46db788 100644
--- a/src/libGLESv2/Program.cpp
+++ b/src/libGLESv2/Program.cpp
@@ -101,22 +101,27 @@ void InfoLog::append(const char *format, ...)
     va_end(vararg);

     char *logPointer = NULL;
+    size_t logLength = 0;

     if (!mInfoLog)
     {
         mInfoLog = new char[infoLength + 2];
         logPointer = mInfoLog;
+        logLength = infoLength + 2;
     }
     else
     {
         size_t currentlogLength = strlen(mInfoLog);
-        char *newLog = new char[currentlogLength + infoLength + 2];
-        strcpy(newLog, mInfoLog);
+        size_t newInfoLogLength = currentlogLength + infoLength + 2;
+        char *newLog = new char[newInfoLogLength];
+        strncpy(newLog, mInfoLog, newInfoLogLength);
+        newLog[newInfoLogLength - 1] = '\0';

         delete[] mInfoLog;
         mInfoLog = newLog;

         logPointer = mInfoLog + currentlogLength;
+        logLength = newInfoLogLength - currentlogLength;
     }

     va_start(vararg, format);
@@ -124,7 +129,8 @@ void InfoLog::append(const char *format, ...)
     va_end(vararg);

     logPointer[infoLength] = 0;
-    strcpy(logPointer + infoLength, "\n");
+    strncpy(logPointer + infoLength, "\n", logLength - infoLength);
+    logPointer[logLength - 1] = '\0';
 }

 void InfoLog::reset()

Original comment by jmad...@chromium.org on 5 May 2014 at 2:18

• Changed state: Started

[GoogleCodeExporter]

https://chromium-review.googlesource.com/#/c/198182/

Original comment by jmad...@chromium.org on 5 May 2014 at 2:22

• Changed state: Review

[GoogleCodeExporter]

> Did the strcpy fixes ever make it into ANGLE?

No, that's why this bug is still open.

In general, changes to ANGLE by Apple employees need to be upstreamed by a 
Chromium or Blink engineer under the same BSD license used to commit the 
changes to the WebKit source repository.

Original comment by ddkilzer@gmail.com on 6 May 2014 at 11:30