Bug 129237 - ANGLE: Stop using unsafe strcpy method
Summary: ANGLE: Stop using unsafe strcpy method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL: https://code.google.com/p/angleprojec...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-23 15:55 PST by David Kilzer (:ddkilzer)
Modified: 2014-02-24 03:09 PST (History)
5 users (show)

See Also:


Attachments
Patch v1 (5.13 KB, patch)
2014-02-23 16:13 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (5.35 KB, patch)
2014-02-23 16:27 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (4.74 KB, patch)
2014-02-23 16:52 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (4.79 KB, patch)
2014-02-23 17:25 PST, David Kilzer (:ddkilzer)
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2014-02-23 15:55:41 PST
ANGLE should stop using the unsafe strcpy method.
Comment 1 David Kilzer (:ddkilzer) 2014-02-23 15:56:28 PST
<rdar://problem/11077580>
Comment 2 David Kilzer (:ddkilzer) 2014-02-23 16:13:46 PST
Created attachment 225016 [details]
Patch v1
Comment 3 WebKit Commit Bot 2014-02-23 16:15:53 PST
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 WebKit Commit Bot 2014-02-23 16:15:59 PST
Attachment 225016 [details] did not pass style-queue:


ERROR: Source/ThirdParty/ANGLE/src/libGLESv2/Program.cpp:118:  Declaration has space between type name and * in char *newLog  [whitespace/declaration] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 David Kilzer (:ddkilzer) 2014-02-23 16:19:11 PST
Comment on attachment 225016 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=225016&action=review

> Source/ThirdParty/ANGLE/src/common/angleutils.h:47
> +#define strlcpy(dst, src, dstsize) strcpy_s(dst, dstsize, src)

Waiting for the EWS bots to tell me if this works on Windows.

> Source/ThirdParty/ANGLE/src/compiler/ShaderLang.cpp:226
> +    size_t infoLogLength = 0;
> +    ShGetInfo(compiler, SH_INFO_LOG_LENGTH, &infoLogLength);

A better design would be to pass in the buffer length to ShGetInfoLog() as a parameter, but since it's a C API, a new function name would have to be created.

> Source/ThirdParty/ANGLE/src/compiler/ShaderLang.cpp:245
> +    size_t objCodeLength = 0;
> +    ShGetInfo(handle, SH_OBJECT_CODE_LENGTH, &objCodeLength);

Ditto for passing in the length to ShGetObjectCode().

> Source/ThirdParty/ANGLE/src/libGLESv2/Program.cpp:112
> +        const size_t newInfoLogLength = infoLength + 2;
> +        mInfoLog = new char[newInfoLogLength];
> +        strlcpy(mInfoLog, info, newInfoLogLength);
> +        strlcpy(mInfoLog + infoLength, "\n", newInfoLogLength - infoLength);

I think using snprintf() here would be clearer, but probably not as performant.

> Source/ThirdParty/ANGLE/src/libGLESv2/Program.cpp:121
> +        const size_t newInfoLogLength = logLength + infoLength + 2;
> +        char *newLog = new char[newInfoLogLength];
> +        strlcpy(newLog, mInfoLog, newInfoLogLength);
> +        strlcpy(newLog + logLength, info, newInfoLogLength - logLength);
> +        strlcpy(newLog + logLength + infoLength, "\n", newInfoLogLength - logLength - infoLength);

Ditto for using snprintf().
Comment 6 David Kilzer (:ddkilzer) 2014-02-23 16:27:51 PST
Created attachment 225017 [details]
Patch v2

Try to fix builds by adding #include <string.h> to ShaderLang.cpp.
Comment 7 WebKit Commit Bot 2014-02-23 16:28:32 PST
Attachment 225017 [details] did not pass style-queue:


ERROR: Source/ThirdParty/ANGLE/src/libGLESv2/Program.cpp:118:  Declaration has space between type name and * in char *newLog  [whitespace/declaration] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 David Kilzer (:ddkilzer) 2014-02-23 16:40:24 PST
(In reply to comment #6)
> Created an attachment (id=225017) [details]
> Patch v2
> 
> Try to fix builds by adding #include <string.h> to ShaderLang.cpp.

Hmm, strlcpy doesn't seem to be used in WebCore, so we should probably use strncpy with an explicit '\0' set at the end.
Comment 9 David Kilzer (:ddkilzer) 2014-02-23 16:52:54 PST
Created attachment 225018 [details]
Patch v3

Switch to strncpy() instead of strlcpy().
Comment 10 David Kilzer (:ddkilzer) 2014-02-23 17:25:17 PST
Created attachment 225020 [details]
Patch v4

Update patch after Dean landed updated ANGLE in r164565, r164567.
Comment 11 WebKit Commit Bot 2014-02-23 17:26:41 PST
Attachment 225020 [details] did not pass style-queue:


ERROR: Source/ThirdParty/ANGLE/src/libGLESv2/Program.cpp:119:  Declaration has space between type name and * in char *newLog  [whitespace/declaration] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Dean Jackson 2014-02-23 17:28:18 PST
Comment on attachment 225020 [details]
Patch v4

We should file a bug on ANGLE to pick this up. I'll do so.
Comment 13 David Kilzer (:ddkilzer) 2014-02-23 17:29:20 PST
(In reply to comment #12)
> (From update of attachment 225020 [details])
> We should file a bug on ANGLE to pick this up. I'll do so.

https://code.google.com/p/angleproject/issues/detail?id=307
Comment 14 David Kilzer (:ddkilzer) 2014-02-24 03:09:22 PST
Committed r164580: <http://trac.webkit.org/changeset/164580>