search

Warzone2100 / old-trac-import

Archived Import of (old) Warzone 2100 Trac
0 stars 0 forks source link

psStat checks to help trackdown corrupted pointers #1903

Closed wzdev-ci closed 14 years ago

wzdev-ci commented 14 years ago

resolution_fixed type_patch (an actual patch, not a request for one) | by Crymson

Hope I followed the rules correctly for the patch.

I added code to assert when bad pointers would have been made, and fixed a few other asserts to return instead of happily continuing on, as if the pointer wasn't bad.

see ticket 1900 for how it will help that guy out.

Issue migrated from trac:1903 at 2022-04-15 22:00:45 -0700

wzdev-ci commented 14 years ago

Crymson uploaded file psStatChecks.patch (34.4 KiB)

wzdev-ci commented 14 years ago

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...

wzdev-ci commented 14 years ago

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.

wzdev-ci commented 14 years ago

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...

wzdev-ci commented 14 years ago

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.

wzdev-ci commented 14 years ago

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.

wzdev-ci commented 14 years ago

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.

wzdev-ci commented 14 years ago

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.

wzdev-ci commented 14 years ago

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]

  1. 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.

wzdev-ci commented 14 years ago

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.

wzdev-ci commented 14 years ago

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

Version without pure whitespace changes, v2.

wzdev-ci commented 14 years ago

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?).

wzdev-ci commented 14 years ago

cybersphinx uploaded file stats.patch (26.8 KiB)

Possibly correct. The asserts messages could still be shortened.

wzdev-ci commented 14 years ago

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.

wzdev-ci commented 14 years ago

Buginator _uploaded file stats_svn2a.patch (26.3 KiB)_

wzdev-ci commented 14 years ago

Buginator _uploaded file stats_svn2a-trunk.patch (24.8 KiB)_

wzdev-ci commented 14 years ago

Buginator changed status from new to closed

wzdev-ci commented 14 years ago

Buginator set resolution to fixed

wzdev-ci commented 14 years ago

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]

wzdev-ci commented 14 years ago

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]

wzdev-ci commented 14 years ago

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]

wzdev-ci commented 14 years ago

Git SVN Gateway gateway@... commented

(In https://github.com/Warzone2100/warzone2100/commit/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

wzdev-ci commented 13 years ago

Git SVN Gateway gateway@... commented

In https://github.com/Warzone2100/warzone2100/commit/abd47389285f6b80a7406e029d6f1fca9763374a:

#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
wzdev-ci commented 13 years ago

Git SVN Gateway gateway@... commented

In https://github.com/Warzone2100/warzone2100/commit/abd47389285f6b80a7406e029d6f1fca9763374a:

#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