1.30.x era versions break back-compat with ConvertAttackType(7) and GetUnitArmorType - AGAIN

[Cokemonkey11]

Old thread: 1.28.x era versions break back-compat with ConvertAttackType(7) and GetUnitArmorType

Other examples of use: [vJASS] - DamagePackage , Damage Type ? , [code=jass] - Hidden ATTACK_TYPE in the World Editor?

Unfortunately, this time it seems worse than last time: my brief tests so far indicate that dealing 1 damage with ConvertAttackType(7) will do either 0 or 0.001 damage.

*To be clear: the issue in question is that this breaks compatibiliy for old maps that use this behavior*.

 

[DracoL1ch]

this behavior based on the way memory layed out, therefore no doubts it would be fucked by literally any update

 

[Cokemonkey11]

this behavior based on the way memory layed out, therefore no doubts it would be fucked by literally any update

"Jass natives aren't idempotent"

That's great, thanks for your input

 

[IcemanBo]

But don't you ignore it's not even listed by blizzard? It might point on some arbitary written memory, just falsly interpreting parts of it as part of attacktype struct. Seems like hack to rely on it.

 

[Cokemonkey11]

But don't you ignore it's not even listed by blizzard? It might point on some arbitary written memory, just falsly interpreting parts of it as part of attacktype struct. Seems like hack to rely on it.

- ConvertAttackType is "listed by blizzard". It's a native.
- This behavior was consistent for at least 7 years
- There are maps that use this behavior, and blizzard's aim was was to ensure old maps are fully compatible.

Frankly, I don't really care about the implentation details.

There was an API.

The API behavior worked consistently for years.

The API behavior changed with an update.

 

[IcemanBo]

Even ConvertAttackType() is listed as native to retrieve enum... shit in, shit out, isn't it?
I mean I don't know it, but if a pointer really just points randomly on some not-0 values, interpreting it as attacktype... then I see no point on arguing it is a real bug, only because it became useful. It might break using a method, but imo calling jass natives not consistent solely of maybe not even wanted or defined values seems a bit unfair, too.

 

[Cokemonkey11]

Even ConvertAttackType() is listed as native to retrieve enum... shit in, shit out, isn't it?
I mean I don't know it, but if a pointer really just points randomly on some not-0 values, interpreting it as attacktype... then I see no point on arguing it is a real bug, only because it became useful. It might break using a method, but imo calling jass natives not consistent solely of maybe not even wanted or defined values seems a bit unfair, too.

Where is it documented as "retrieves enum"?

there are no pointers in jass

there is no implicit conversion/interpreting

We are just using an API that has very poor type-safety

 

[IcemanBo]

attacktype seems like enum, or struct behind
c++/c has pointers, jass is exposed functionality
accessing invalid attackindex might be seen like accessing not forseen part in memory, -> being interpreted as valid attacktype, even no intention

Yes using the API might have security lacks, but that doesn't strengthen your point of claiming them breaking compatibility.
You can, or at least you could use negative values in TriggerSleepAction(), uo to -121 or, so and it worked same as TriggerSleepAction(0). Seeminlgy arbitary, as lower values completly crash the thread from executing. Will you come now in a patch and blame them to break compatibitly if TriggerSleepAction(-10) doesn't work anymore same as using "0"? It's just wrong usage, and in case you lost benefits from accidently patched out features it's just bad luck...

 

[Cokemonkey11]

attacktype seems like enum, or struct behind

So what?

c++/c has pointers, jass is exposed functionality

Are you trying to tell me that it's good design/okay for jass to expose pointers and pointer capabilities!?

accessing invalid attackindex might be seen like accessing not forseen part in memory, -> being interpreted as valid attacktype, even no intention

I don't care what it might be seen as, because my argument is about something fundamental: functional idempotence

Yes using the API might have security lacks, but that doesn't strengthen your point of claiming them breaking compatibility.

This hasn't to do with security: it's API design 101

You can, or at least you could use negative values in TriggerSleepAction(), uo to -121 or, so and it worked same as TriggerSleepAction(0). Seeminlgy arbitary, as lower values completly crash the thread from executing. Will you come now in a patch and blame them to break compatibitly if TriggerSleepAction(-10) doesn't work anymore same as using "0"? It's just wrong usage, and in case you lost benefits from accidently patched out features it's just bad luck...

No, that's also a bug. If my map used it, I would equally complain. However, as I don't know of anyone using TSA with a negative value, I wouldn't raise this as a major flaw.

OTOH I provided multiple direct references to people talking about ConvertAttackType(7)

 

[IcemanBo]

Are you trying to tell me that it's good design/okay for jass to expose pointers and pointer capabilities!?

I imagine it similar like trying to read memory out of array index in c++. It will just read memory with certain infex-offset, and no index-bounds check is done here. Usually there will be written shit or nothing in the memory, if the index is "out of bounds", but with some luck you might get an memory offset with a special index which will lead to some memory that can be interpreted as valid array element again, like the attack type. This would be just random scenario, and undefined behaviour in native c++.

 

[Cokemonkey11]

I imagine it similar like trying to read memory out of array index in c++. It will just read memory with certain infex-offset, and no index-bounds check is done here. Usually there will be written shit or nothing in the memory, if the index is "out of bounds", but with some luck you might get an memory offset with a special index which will lead to some memory that can be interpreted as valid array element again, like the attack type. This would be just random scenario, and undefined behaviour in native c++.

1. I'm very familiar with C++ and I made the some conclusions you did

2. that's shitty C++

3. I don't care either about the technical explanation, nor the presumed mental mode.

4. It's wrong. It's garbage. It broke backwards-compatibility.

 

[DracoL1ch]

go tell the compiler to put some mem allocs strictly one after another

 

[Cokemonkey11]

go tell the compiler to put some mem allocs strictly one after another

1. I wouldn't have guessed that this is a problem of compiler non-determinism. I would have guessed that this related to either (a) using calloc instead of malloc, (b) using newer compiler, or (c) using a different compiler

2. API correctness and consistency is the job of the author, not the compiler

3. I'm not interested in the technical reason

4. It's well-within the game author's power to (a) fix this (b) provide natives that enable a replacement for this use-case, and (c) test better

 

[DracoL1ch]

unlike windows, wc3 isnt billion-users-wide system and doesn't have to preserve all kind of strange interactions. I understand why it's broken now and why it might be hard to fix, but it wasn't meant to stay neither

 

[Cokemonkey11]

unlike windows, wc3 isnt billion-users-wide system and doesn't have to preserve all kind of strange interactions.

"consistency guarantees are a problem for big companies and we can afford to be sloppy"

I understand why it's broken now and why it might be hard to fix, but it wasn't meant to stay neither

You do? Please, explain why it would be hard to fix

 

[IcemanBo]

Don't you think you're being a bit ignorant with calling it broken compatability, when, again, it's maybe no planned functionality?
One can argue the same for any bug fix, which changes behaviour. Someone could rely on weird behaviour in some way, and fixed one could be called "broken".

Honest question, if they fixed it now, not allowing ConvertAttackType() to get values out of bounds of listed attack types - it would "break" backwards the same way. Would it make a better situation? One could shout at them the same way, blaiming to return now null values instead of arbiatry ones, breaking our usage.

If suggestion is marely to provide alternative solution I can very understand it, but when reading it mostly seems like they did just wrong again, "I don't care what reason there is, but I want old usage back, idc if it was a bug or not", which seems a bit unfair and pointless fight.

 

[Cokemonkey11]

Don't you think you're being a bit ignorant with calling it broken compatability, when, again, it's maybe no planned functionality?
One can argue the same for any bug fix, which changes behaviour. Someone could rely on weird behaviour in some way, and fixed one could be called "broken".

I disagree. I have two criterium:

1: behavior changed from an old version to the current one

2: there was clearly someone using the old behavior

Honest question, if they fixed it now, not allowing ConvertAttackType() to get values out of bounds of listed attack types - it would "break" backwards the same way.

Are you saying they would fix ConvertAttackType to perform bounds checking, or are you saying they would revert the change so that ConvertAttackType(7) behaves as in older versions?

In the first case: Objection, zero-sum fallacy.

In the second case: see (2) above

Would it make a better situation?One could shout at them the same way, blaiming to return now null values instead of arbiatry ones, breaking our usage.

As above.

If suggestion is marely to provide alternative solution I can very understand it, but when reading it mostly seems like they did just wrong again, "I don't care what reason there is, but I want old usage back, idc if it was a bug or not", which seems a bit unfair and pointless fight.

I'm not suggesting anything in particular. I trust blizzard to work that out for themselves. I'm reporting that there exists a timeline like this:

- blizzard says they won't break backwards compatibility

- blizzard broke something

Do I think it's reasonable that blizzard didn't catch this already? Yes, absolutely. It's still broken.

 

[IcemanBo]

Yes, blizzard don't ever make any changes that fix behaviour, no matter if it's fixing bugs or not. Please.

edit:

Against my opinion, I have read more about similar issues in the internet, and I found out even it might be not best, there are cases where a company willingly holds bugged behaviour for being backwards-compatible. Microsoft Excel had a famous leap-year bug with year 1900, which has lead to two date modes which could be chosen from then on. One fixed mode with correct date, and one old and broken date mode, that still can be used, too, for staying backwards compatible.

Intuitively I still would think the opposite, not to allow such things, but obviously there are also cases who share your opinion.

 

[Bribe]

Just use ConvertDefenseType(BlzGetUnitIntegerField(whichUnit, UNIT_IF_DEFENSE_TYPE)) and compare it against the following:

constant defensetype DEFENSE_TYPE_LIGHT = ConvertDefenseType(0)
constant defensetype DEFENSE_TYPE_MEDIUM = ConvertDefenseType(1)
constant defensetype DEFENSE_TYPE_LARGE = ConvertDefenseType(2)
constant defensetype DEFENSE_TYPE_FORT = ConvertDefenseType(3)
constant defensetype DEFENSE_TYPE_NORMAL = ConvertDefenseType(4)
constant defensetype DEFENSE_TYPE_HERO = ConvertDefenseType(5)
constant defensetype DEFENSE_TYPE_DIVINE = ConvertDefenseType(6)
constant defensetype DEFENSE_TYPE_NONE = ConvertDefenseType(7)

Also the Return Bug was a big regression for many maps, and people hated it, but this is a very simple - and much more efficient - fix.

 

[Cokemonkey11]

Yes, blizzard don't ever make any changes that fix behaviour, no matter if it's fixing bugs or not. Please.

edit:

Against my opinion, I have read more about similar issues in the internet, and I found out even it might be not best, there are cases where a company willingly holds bugged behaviour for being backwards-compatible. Microsoft Excel had a famous leap-year bug with year 1900, which has lead to two date modes which could be chosen from then on. One fixed mode with correct date, and one old and broken date mode, that still can be used, too, for staying backwards compatible.

Intuitively I still would think the opposite, not to allow such things, but obviously there are also cases who share your opinion.

What bugs are you talking about?

> Just use ConvertDefenseType(BlzGetUnitIntegerField(whichUnit, UNIT_IF_DEFENSE_TYPE)) and compare it against the following:

Thanks. I've just published a wrapper around this for wurst here: Cokemonkey11/WurstArmorType

> Also the Return Bug was a big regression for many maps, and people hated it, but this is a very simple - and much more efficient - fix.

The return bug was an enormous security hole, and there was no public statement from blizzard just months beforehand promising not to break existing maps.

With 1.31, that was the case.