Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UPBGE: Fix for issue #797 #869

Merged
merged 1 commit into from
Oct 9, 2018
Merged

UPBGE: Fix for issue #797 #869

merged 1 commit into from
Oct 9, 2018

Conversation

lordloki
Copy link
Member

@lordloki lordloki commented Oct 4, 2018

Set GPU_INT to 5 avoid buffer overrun

This is due to the following:
in gpu_codegen.c in gpu_node_input_link we have the following catch all

else {
		/* uniform vector */
		input->type = type;
		input->source = GPU_SOURCE_VEC_UNIFORM;

		memcpy(input->vec, link->ptr1, type * sizeof(float)); <<---- for int = 17 * float
		if (link->dynamic) {
			input->dynamicvec = link->ptr1;
			input->dynamictype = link->dynamictype;
			input->dynamicdata = link->ptr2;
		}
		MEM_freeN(link);
	}

and in the typedef for GPUInput we have this for vec

float vec[16];               /* vector data */
GPUNodeLink *link;

As the type for INT was 17, this will copy 17 floats from link->ptr1 overwriting input->link with garbage.

@lordloki lordloki requested a review from panzergame October 4, 2018 21:50
@@ -1297,7 +1297,13 @@ static void gpu_node_input_link(GPUNode *node, GPUNodeLink *link, const GPUType
input->type = type;
input->source = GPU_SOURCE_VEC_UNIFORM;

memcpy(input->vec, link->ptr1, type * sizeof(float));
if (type == GPU_INT) {
Copy link
Member Author

@lordloki lordloki Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@panzergame We can even make this GPU_INT check only and left all above changes as they are in current master

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using an array for size of types indexed by GPUType ? So you can change the values and avoid exceptions in the code when adding conditions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced to do that before final release. I would prefer fix memcpy and make the tiny refactor after

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you want, but the refactor is just defining an array of 18 elements.

@lordloki lordloki added the bug label Oct 5, 2018
@lordloki lordloki force-pushed the ge_bug_fix_797_alternative branch from eb06a57 to 637bee8 Compare October 6, 2018 17:03
@@ -55,6 +55,11 @@
#include <string.h>
#include <stdarg.h>

#ifndef ARRAYSIZE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to include BLI_utildefines.h for this macro ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done

Copy link
Contributor

@panzergame panzergame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just squash and make a commit message, then push on master. Thank you for the patch.

Introduce a new array of GPUType sizes to avoid a buffer overrun.
The new array will allow make the system extensible easier.
More comments added.

P.D. It fixes the win32 crash
@lordloki lordloki force-pushed the ge_bug_fix_797_alternative branch from e46fe8f to d438f1c Compare October 9, 2018 22:23
@lordloki lordloki merged commit 683c104 into master Oct 9, 2018
@lordloki lordloki deleted the ge_bug_fix_797_alternative branch October 10, 2018 07:18
youle31 pushed a commit that referenced this pull request May 26, 2019
Introduce a new array of GPUType sizes to avoid a buffer overrun when we introduce new elements in GPUType.
The new array will allow make the system extensible easier.
More comments added.

P.D. It fixes the win32 crash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants