Crymson uploaded file psStatChecks.patch (34.4 KiB)

cybersphinx commented

Well, the patch includes some reindentation, if that's needed it should be separated from code changes. And in some places you use tabs to align things (which breaks with anything except your tab setting), those should be spaces.

All those places where you add compIndex look like they could be put into a function.

Also, it would be nice to have a patch in the ticket with the problem it addresses.

For the problem itself, I guess your patch works around it, though somewhere the template list gets corrupted, which might lead to other problems later...

cybersphinx commented

Hm, actually trying to compile I had to remove the assert in scriptfuncs.c:9016 and the change to sstrcpy, since those lead to warnings. Also, the game saves, but the newly created template isn't there. Though I guess that narrows things down a bit.

cybersphinx commented

Looks like only loading the last saved template fails in game.c:9185, though I'm not sure why we'd want to output SDWORD psSaveTemplate->asParts[i] as a string there...

Zarel commented

The compIndex stuff should definitely be going into a function.

inline WEAPON_STATS* getWeaponStats(BASE_OBJECT* psObj, int i);

or something like that.

Crymson commented

Replying to Warzone2100/old-trac-import#1903 (comment:4):

The compIndex stuff should definitely be going into a function.

That will not help find the problem, which is the main reason for doing it the way I did it.

For example, let us say it is wrapped in a function, and we hit it. On debug builds, that is OK, since we can check the stack and see where we are coming from. On release builds, then we have no clue, and just get the assert message from the wrapped function, and not the routine where the problem occurred. In other words, it adds another hidden layer if you use a function instead of the way it currently is in the patch.

Cybersphinx, what warning did sstrcpy() cause?
For the formatting, I was in that function, and my editor auto-adjusted, since it noticed the conflicting styles. I don't know why this wasn't fixed before.

Zarel commented

Replying to Warzone2100/old-trac-import#1903 (comment:5):

That will not help find the problem, which is the main reason for doing it the way I did it.
Yes it will.

For example, let us say it is wrapped in a function, and we hit it. On debug builds, that is OK, since we can check the stack and see where we are coming from.
On release builds, then we have no clue, and just get the assert message from the wrapped function, and not the routine where the problem occurred.

1. Even if there is no debug information, since the function is inline, the message given will be from the routine that called the function, not the function itself.

2. On release builds, there are no assert messages at all.

In other words, it adds another hidden layer if you use a function instead of the way it currently is in the patch.

In other words, no, it doesn't.

cybersphinx commented

Replying to Warzone2100/old-trac-import#1903 (comment:5):

For example, let us say it is wrapped in a function, and we hit it.

Could use a macro then.

Cybersphinx, what warning did sstrcpy() cause?

../../src/game.c:11613: error: negative width in bit-field ‘’

Don't ask me what that means, I just looked at the line, saw that it was one you changed, and changed it back.

Crymson commented

Replying to Warzone2100/old-trac-import#1903 (comment:6):

Replying to Warzone2100/old-trac-import#1903 (comment:5):

That will not help find the problem, which is the main reason for doing it the way I did it.
Yes it will.

O_o
[facepalm]

For example, let us say it is wrapped in a function, and we hit it. On debug builds, that is OK, since we can check the stack and see where we are coming from.
On release builds, then we have no clue, and just get the assert message from the wrapped function, and not the routine where the problem occurred.

1. Even if there is no debug information, since the function is inline, the message given will be from the routine that called the function, not the function itself.

O_o
[facepalm x2]

2. On release builds, there are no assert messages at all.
   O_o
   [facepalm x3]

In other words, it adds another hidden layer if you use a function instead of the way it currently is in the patch.

In other words, no, it doesn't.

Dude, if you don't want to use the patch, then fine, I was only trying to help.

In the future, it might help you to brush up on how preprocessors work, since it is painfully obvious that you haven't used them before.
Suggesting to use a inline function, in this case, is farcical, for the reasons I stated.
Sorry if that is harsh, but I don't see another way to put it.
I hope this isn't because I suggested that one of your patches needed to be reverted.

Cybersphinx, that is a odd error message, what compiler are you using?
Using macros is very messy in this case, it would involve anonymous code blocks, and skews the error reporting lines, but, if you wish, you may change it how you see fit.

There are also some posts about patches in the forums that you guys didn't apply for various reasons, so it seems that your wiki needs to be updated to make it more clear. It would save a lot of time.

Zarel commented

Replying to Warzone2100/old-trac-import#1903 (comment:8):

1. Even if there is no debug information, since the function is inline, the message given will be from the routine that called the function, not the function itself.

O_o
[facepalm x2]

Oh! I see what you mean; you're referring to the function name in the debug log. Yeah, you're right, an inline function would interfere with that. We could still use a #define, though the way I see it being set up, the ASSERT would occur outside of the inline function anyway.

Dude, if you don't want to use the patch, then fine, I was only trying to help.

I don't know how suggesting improvements to your patch in any way implies I don't want it to be committed.

In the future, it might help you to brush up on how preprocessors work, since it is painfully obvious that you haven't used them before.
Suggesting to use a inline function, in this case, is farcical, for the reasons I stated.

The reasons you've stated are "facepalm" and "O_o". It's a good thing I managed to figure out that you were talking about the error log; next time, English would be preferable for communication.

Sorry if that is harsh, but I don't see another way to put it.
I hope this isn't because I suggested that one of your patches needed to be reverted.

Psh, I'm not that petty.

cybersphinx uploaded file psStatChecks-cleaned.patch (27.0 KiB)

Version without pure whitespace changes, v2.

cybersphinx commented

Compiler is gcc 4.4.4 on Linux. For the ASSERT_OR_RETURN in src/scriptfuncs.c:9016, I guess this was supposed to use psStruct instead of psDroid? The one above that looks also strange. Why do you compare some droid stats to numStructureStats? Also, you don't need to put the asserted condition into the text, it is printed automatically (or is that different on Windows?).

cybersphinx uploaded file stats.patch (26.8 KiB)

Possibly correct. The asserts messages could still be shortened.

Buginator commented

Slight updates, also made it a bit more merge friendly for 2.3, though, that doesn't have to be there, it will just save time in the future.

Buginator uploaded file stats_svn2a.patch (26.3 KiB)

Buginator uploaded file stats_svn2a-trunk.patch (24.8 KiB)

Buginator changed status from new to closed

Buginator set resolution to fixed

Buginator commented

(In [11061]) Add bounds checking to psStats to prevent bad pointers.
Thanks to Crymson for the original patch, with modifications by Cybersphinx & Buginator.

Closes #1903
2.3: [11059]

Buginator commented

(In [11062]) Add bounds checking to psStats to prevent bad pointers.
Thanks to Crymson for the original patch, with modifications by Cybersphinx & Buginator.

Closes #1903
2.3: [11059]

Buginator commented

(In [11063]) Add bounds checking to psStats to prevent bad pointers.
Thanks to Crymson for the original patch, with modifications by Cybersphinx & Buginator.

Closes #1903
2.3: [11059]

Git SVN Gateway <gateway@...> commented

(In Warzone2100/warzone2100@abd4738) Add bounds checking to psStats to prevent bad pointers.
Thanks to Crymson for the original patch, with modifications by Cybersphinx & Buginator.

Closes #1903
2.3: [11059]

git-svn-id: https://warzone2100.svn.sourceforge.net/svnroot/warzone2100/trunk@11061 4a71c877-e1ca-e34f-864e-861f7616d084

Git SVN Gateway <gateway@...> commented

In Warzone2100/warzone2100@abd4738:

#CommitTicketReference repository="" revision="abd47389285f6b80a7406e029d6f1fca9763374a"
Add bounds checking to psStats to prevent bad pointers.
Thanks to Crymson for the original patch, with modifications by Cybersphinx & Buginator.

Closes #1903
2.3: [11059]

git-svn-id: https://warzone2100.svn.sourceforge.net/svnroot/warzone2100/trunk@11061 4a71c877-e1ca-e34f-864e-861f7616d084