| Summary: | Incorrect/undefined behavior from shifting an integer too far | ||
|---|---|---|---|
| Product: | Mesa | Reporter: | Bruce Dawson <brucedawson> |
| Component: | Drivers/DRI/swrast | Assignee: | mesa-dev |
| Status: | RESOLVED MOVED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | ||
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | patch to fix the issues in s_span.c | ||
(In reply to Bruce Dawson from comment #0) > drivers\dri\i965\brw_fs.cpp(2164): > for (int i = 0; i < FRAG_ATTRIB_MAX; i++) { > if (!(fp->Base.InputsRead & BITFIELD64_BIT(i))) > continue; > > if (prog->Name == 0) > key.proj_attrib_mask |= 1 << i; // BUG This doesn't exist in the code base. In fact, the proj_attrib_mask field was removed in April 2013! (commit 705c8247) I'm marking as WONTFIX, because there's no ALREADYFIX and bugzilla isn't configured to allow OBSOLETE statuses. I haven't checked whether the other two are still present. Please just submit fixes directly to the mailing list. Created attachment 110683 [details] [review] patch to fix the issues in s_span.c Hi Bruce! Long time no see (err, email). Here's a patch which I believe will fix the issues in s_span.c. I think I've actually seen warnings about that in the past. But the swrast code is pretty old and not used much anymore so it hasn't been a high priority. Can you apply the patch and see if that resolves the warnings you saw? Thanks. (In reply to Brian Paul from comment #2) > Created attachment 110683 [details] [review] [review] > patch to fix the issues in s_span.c > > Hi Bruce! Long time no see (err, email). > > Here's a patch which I believe will fix the issues in s_span.c. I think > I've actually seen warnings about that in the past. But the swrast code is > pretty old and not used much anymore so it hasn't been a high priority. > > Can you apply the patch and see if that resolves the warnings you saw? > > Thanks. Hi Brian -- last seen in Hawaii I believe? Small world. Our version of mesa seems to be extremely old, predating the rename from FRAG_ATTRIB_MAX to VARYING_SLOT_MAX, so I can't apply the patch, but I took a look at it. I don't understand the first fix where you change the loop range to SPAN_NUM_ATTRIBS. That seems orthogonal. It seems that the (1 << i) still needs updating. The second fix looks good. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/314. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
While compiling Google Chrome with VC++'s /analyze (static code analysis) I discovered a class of bugs in the mesa code base. There are three instances of this bug where the integer '1' is shifted by an amount that ranges from zero to 47. '1' will have type 'int' and on basically every modern platform 'int' is 32 bits. Shifting an int more than 30 positions leads to undefined behavior because the result will, necessarily be unrepresentable. Even if we ignore the annoying spectre of undefined behavior, the behavior will most definitely not be what is intended. On Intel processors the likely result is going to be equivalent to shifting by shiftAmount&31 which means that 16 mask values will be repeated. The warning is: warning C6297: Arithmetic overflow: 32-bit value is shifted, then cast to 64-bit value. Results might not be an expected value. The fix is to use BITFIELD64_BIT(i) instead of (1 << I). The locations where I have noticed this bug are: drivers\dri\i965\brw_fs.cpp(2164): for (int i = 0; i < FRAG_ATTRIB_MAX; i++) { if (!(fp->Base.InputsRead & BITFIELD64_BIT(i))) continue; if (prog->Name == 0) key.proj_attrib_mask |= 1 << i; // BUG swrast\s_span.c(767): for (i = 0; i < FRAG_ATTRIB_MAX; i++) { if (span->interpMask & (1 << i)) { GLuint j; for (j = 0; j < 4; j++) { span->attrStart[i][j] += leftClip * span->attrStepX[i][j]; } } } swrast\s_span.c(788): for (i = 0; i < FRAG_ATTRIB_MAX; i++) { if (span->arrayAttribs & (1 << i)) { /* shift array elements left by 'leftClip' */ SHIFT_ARRAY(span->array->attribs[i], leftClip, n - leftClip); } }