shadeinput.c and zbuf.c... out of bounds memory indexing? #44033

New Issue

Michael Carroll · 2015-03-18T01:31:18+01:00

Michael Carroll commented

2015-03-18 01:31:18 +01:00

System Information
Win32 Win8 x64 x86_64

Blender Version
Observed Broken: 485293647f Tue Oct 28 21:39:54 2014 +0100
Appears Broken: 63897304a9 Wed Mar 18 00:09:54 2015 +0500
Worked: Never?

Win32 blender showed artifacts across the rendering of a cube face against the default cube input. I took intel memory checker to my binary in windows. This code appeared to be addressing memory outside of an the array centLut, when centLut serves as an input to variable ys:

./source/blender/render/intern/source/shadeinput.c:

                              if (R.osa) {
                                      short b = R.samples->centmask[curmask];
                                      xs = (float)x + R.samples->centLut[b & 15] + 0.5f;
                                      ys = (float)y + R.samples->centLut[b >> 4] + 0.5f;
                              }

Upon a forcing the indexer to be 0 <-> 15 size the artifact was alleviated:

                              if (R.osa) {
                                      short b = R.samples->centmask[curmask];
                                      xs = (float)x + R.samples->centLut[b & 15] + 0.5f;
                                      ys = (float)y + R.samples->centLut[(b >> 4) & 15] + 0.5f;
                              }

centLut is defined to be 16 floats:
./source/blender/render/intern/include/render_types.h:73:

     float centLut[16];

centmask is defined to be 256 chars but a short is being used to derive an index from it
./source/blender/render/intern/include/render_types.h:75:

char cmask[256], *centmask;

So short b shifted 4 bits can be outside of the domain of the array and in my case it picked up unintended values. There may be a similar issue seen at ./source/blender/render/intern/source/zbuf.c:3830

Exact steps for others to reproduce the error
I may have been the one of the only people to see this error because I am running an extremely stripped down version of blender. The build has a minimal featureset as I specified by cmake. From there I have manually taken out many many dependencies. But I simply:

Set up the default cube into a blend file
Rendered it in background mode on windows:

blender.exe cube.blend --render-output cube_ --threads 1 -b -F RAWTGA -s 1 -e 1 -a

Observed my render... which showed artifacts accross triangle seams of the cube. This occured 40% of the time I ran it. Seams were darker than expected.

Hopefully this is a simple fix.

Thanks,

MichaelC

**System Information** Win32 Win8 x64 x86_64 **Blender Version** Observed Broken: 485293647ff1a054da6821b09d567ee94a63a7a8 Tue Oct 28 21:39:54 2014 +0100 Appears Broken: 63897304a90a8b13304771af0f466f6d13cb9227 Wed Mar 18 00:09:54 2015 +0500 Worked: Never? Win32 blender showed artifacts across the rendering of a cube face against the default cube input. I took intel memory checker to my binary in windows. This code appeared to be addressing memory outside of an the array centLut, when centLut serves as an input to variable ys: ./source/blender/render/intern/source/shadeinput.c: ``` if (R.osa) { short b = R.samples->centmask[curmask]; xs = (float)x + R.samples->centLut[b & 15] + 0.5f; ys = (float)y + R.samples->centLut[b >> 4] + 0.5f; } ``` Upon a forcing the indexer to be 0 <-> 15 size the artifact was alleviated: ``` if (R.osa) { short b = R.samples->centmask[curmask]; xs = (float)x + R.samples->centLut[b & 15] + 0.5f; ys = (float)y + R.samples->centLut[(b >> 4) & 15] + 0.5f; } ``` centLut is defined to be 16 floats: ./source/blender/render/intern/include/render_types.h:73: ``` float centLut[16]; ``` centmask is defined to be 256 chars but a short is being used to derive an index from it ./source/blender/render/intern/include/render_types.h:75: ``` char cmask[256], *centmask; ``` So short b shifted 4 bits can be outside of the domain of the array and in my case it picked up unintended values. There may be a similar issue seen at ./source/blender/render/intern/source/zbuf.c:3830 **Exact steps for others to reproduce the error** I may have been the one of the only people to see this error because I am running an extremely stripped down version of blender. The build has a minimal featureset as I specified by cmake. From there I have manually taken out many many dependencies. But I simply: - Set up the default cube into a blend file - Rendered it in background mode on windows: ``` blender.exe cube.blend --render-output cube_ --threads 1 -b -F RAWTGA -s 1 -e 1 -a ``` - Observed my render... which showed artifacts accross triangle seams of the cube. This occured 40% of the time I ran it. Seams were darker than expected. Hopefully this is a simple fix. Thanks, - MichaelC

Michael Carroll commented

2015-03-18 01:31:18 +01:00

Changed status to: 'Open'

Michael Carroll commented

2015-03-18 01:31:18 +01:00

Added subscriber: @mcarroll

Michael Carroll commented

2015-03-18 01:32:11 +01:00

Also, the same input file and was run on linux x86_64 against a similarly stripped down blender and never saw an issue!

Thanks.

Also, the same input file and was run on linux x86_64 against a similarly stripped down blender and never saw an issue! Thanks.

Sergey Sharybin was assigned by Bastien Montagne

2015-03-19 12:10:59 +01:00

Bastien Montagne commented

2015-03-19 12:10:59 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2015-03-19 12:10:59 +01:00

Patch seems pretty straight forward, but I do not understand why this is needed - b might be a short, but it is assigned a char, so its upper byte shall always be 0, and b >> 4 shall always be below 16…

Sergey, what do you think?

Patch seems pretty straight forward, but I do not understand why this is needed - b might be a short, but it is assigned a char, so its upper byte shall always be 0, and `b >> 4` shall always be below 16… Sergey, what do you think?

Sergey Sharybin commented

2015-03-19 15:17:25 +01:00

I don't see how it's possible to b to become higher than 255. shifting it by 4 positions right would make it 15 max.

Does the issue happen outside of memory checker? Does it happen with official builds? What compiler you're using?

I don't see how it's possible to `b` to become higher than 255. shifting it by 4 positions right would make it 15 max. Does the issue happen outside of memory checker? Does it happen with official builds? What compiler you're using?

Michael Carroll commented

2015-03-19 19:22:42 +01:00

Artifacts existed with or without the memory checker until the low order bits were masked off. . Compiler is Win8 MSVS 13 update 3 for x86_64bit target.

Wont the short sign extend what ever it picks up from the char? High order bits could be 0b1111 1111.

I can't check against official builds as I don't have the debug facilities set up at the moment. I'm not sure where to get an official build that has the symbols pdb file. The pdb file is required for intel inspector.

In gcc linux x86_64 will sign extend. printf vaargs will then promote to 32bits and sign extend when printing as well... in this case I'd assume MSVS has similar behavior.:

include <stdio.h>

int main()
{

      char data = 0xff;
      short b = data;

      printf("b is: %x, %d\n", b, b);
      return 0;

}

output:
b is: ffffffff, -1

changing char data to 0x7f:

b is: 7f, 127

Artifacts existed with or without the memory checker until the low order bits were masked off. . Compiler is Win8 MSVS 13 update 3 for x86_64bit target. Wont the short sign extend what ever it picks up from the char? High order bits could be 0b1111 1111. I can't check against official builds as I don't have the debug facilities set up at the moment. I'm not sure where to get an official build that has the symbols pdb file. The pdb file is required for intel inspector. In gcc linux x86_64 will sign extend. printf vaargs will then promote to 32bits and sign extend when printing as well... in this case I'd assume MSVS has similar behavior.: # include <stdio.h> int main() { ``` char data = 0xff; short b = data; ``` ``` printf("b is: %x, %d\n", b, b); return 0; ``` } output: b is: ffffffff, -1 changing char data to 0x7f: b is: 7f, 127

Sergey Sharybin commented

2015-03-19 19:48:50 +01:00

char in blender is default to unsigned due to -funsigned-char, so your example is not really correct. If you compile it with the sane cflags as blender it should give you 0xff.

Now, there are loads of areas in blender which depends on this fact so if compiler ignores unsigned nature of char in one place you're really in big trouble. But i also can't reproduce any issues with 32bit blender on linux built with minimal cmake configuration, but i'm on update 2 and official builds are using update 4.

Since it might totally be a compiler issue please test with msvc 2013 update 4.

ADDITION: I've just tested the build again by adding BLI_assert(b >= 0 && b <= 255); to the code and sure enough it never triggered. So if update4 will still be buggy for you it's crucial to know how we can observe something is wrong without having intel memory checker.

char in blender is default to unsigned due to `-funsigned-char`, so your example is not really correct. If you compile it with the sane cflags as blender it should give you 0xff. Now, there are loads of areas in blender which depends on this fact so if compiler ignores unsigned nature of char in one place you're really in big trouble. But i also can't reproduce any issues with 32bit blender on linux built with minimal cmake configuration, but i'm on update 2 and official builds are using update 4. Since it might totally be a compiler issue please test with msvc 2013 update 4. ADDITION: I've just tested the build again by adding `BLI_assert(b >= 0 && b <= 255);` to the code and sure enough it never triggered. So if update4 will still be buggy for you it's crucial to know how we can observe something is wrong without having intel memory checker.

Michael Carroll commented

2015-03-19 21:34:00 +01:00

Thanks for that information. I'll check that my /J flag is hitting every source file and consider moving to update 4 when I can access that system. Apologies for the thrash, after viewing the type differences I thought this would be a quick change.

Sergey Sharybin removed their assignment 2015-03-20 10:47:01 +01:00

Sergey Sharybin commented

2015-03-20 10:47:01 +01:00

Added subscriber: @Sergey

Bastien Montagne commented

2015-03-27 11:15:42 +01:00

Changed status from 'Open' to: 'Archived'

Bastien Montagne closed this issue

2015-03-27 11:15:42 +01:00

Bastien Montagne self-assigned this 2015-03-27 11:15:42 +01:00

Bastien Montagne commented

2015-03-27 11:15:43 +01:00

Eeeeh, no news since over a week… Closing for now, we'll reopen when further info is provided.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

shadeinput.c and zbuf.c... out of bounds memory indexing? #44033

include <stdio.h>