#227 immediate operand size becomes instruction size w/o warning
In an instruction of the following type:
cmp [dwordvariable], byte 0
the instruction size becomes that of the immediate operand,
without emitting a warning about the lack of instruction size. It
should really have to be explicitly coded as
cmp dword [dwordvariable], byte 0
If the programmer has a habit of specifying the operand size instead
of letting the optimizer take care of it, it's thus possible to forget
about the instruction size. This may also happen when changing
from a register operand to an imm8 operand.
The result are hard-to-find bugs because only the first byte of a
word or dword variable is being evaluated. This applies to other
instructions with both imm32/imm16 and imm8 forms as well.
The above should at least emit a warning if not fail outright, which
would however break a lot of existing code, I imagine. Still, that code
needs to be fixed anyway...
Logged In: YES
user_id=804543
The basic problem is that NASM 0.98 doesn't perform
bounds checks on displacements or immediates.
Having added those checks to NASM64, I can say that
doing so was quite a bit of work.
Fundamentally there are three types of checks:
S = -(2^(n-1))...(2^(n-1))-1 e.g. -128...127
U = 0...(2^n)-1 e.g. 0...255
A = -(2^(n-1))...(2^n)-1 e.g. -128...255
If anyone is interested, then I can post the rules
which govern these checks. I've also got a number
of testcases, though they are littered with 64-bit
code that might not be easy to rip out.
Logged In: YES
user_id=804543
To give you an idea, for...
= 0000000012345678h <0> label_bef: equ 12345678h
0000 803E[7856]00 <0> cmp [label_bef],byte 0
0005 803E785600 <0> cmp [12345678h],byte 0
000A 803E[7856]00 <0> cmp [label_aft],byte 0
= 0000000012345678h <0> label_aft: equ 12345678h
...NASM64 says...
z.asm:2: WARNING: displacement is out of word range
input source line: cmp [label_bef],byte 0
final source line: cmp [label_bef],byte 0
z.asm:3: WARNING: displacement is out of word range
input source line: cmp [12345678h],byte 0
final source line: cmp [12345678h],byte 0
z.asm:4: WARNING: displacement is out of word range
input source line: cmp [label_aft],byte 0
final source line: cmp [label_aft],byte 0
Logged In: YES
user_id=642401
Actually, this bug doesn't have anything to do with the address size of the
operation, but with the instruction size itself. The lack of address size checking
is an entirely separate issue.
The bug I reported applies in both 16 and 32 bit mode, probably 64 bit mode as
well, if the memory size of the variable that's referenced is larger than that of the
immediate operand:
cmp [var1],byte 0
var 1: dd 1
nasm assembles the "cmp" as a byte-size instruction without a warning that the
actual instruction size isn't specified, and that it really should be
cmp dword [var1],byte 0
This probably comes from just looking at the size of the operand, such as
cmp [var1],eax
or
cmp [var1],al
I guess the operand being a byte "usually" means that it was a byte register and
so the instruction has to have the size of a byte as well. However, when the
operand is an imm8 that may be sign-extended, that assumption is not correct
and should give a warning instead.
Logged In: YES
user_id=804543
> cmp [var1],byte 0
> var1: dd 1
Other assemblers might perform type checking.
However, NASM does not.
That is, it won't remember that "var1" is of "dword" size.
Fwiw, this is deliberate -- see section 2.2.3 of the manual.
Logged In: YES
user_id=642401
I know nasm doesn't do type checking. However, it shouldn't assume an
instruction size from the immediate operand either.
If I say
cmp [var1],0
I get a warning "Operation size not specified", and that's what I expect, but if I
say
cmp [var1],byte 0
I don't get a warning and the operation become byte sized implicitly, even
though it could be either byte, word or dword and still match an imm8 operand.
Logged In: YES
user_id=804543
> [NASM] shouldn't assume an instruction size from
> the immediate operand
The following insns.dat entries are the culprits:
CMP mem,imm8 \300\1\x80\207\21 8086,SM
CMP mem,imm16 \320\300\134\1\x81\207\131 8086,SM
CMP mem,imm32 \321\300\144\1\x81\207\141 386,SM
Similar entries exist for other instructions in
the 80h/81h/82h/83h, C6h/C7h, and F6h/F7h range.
Grep for "imm[8|16|32]" then "mem" to find them.
Now, since removing those entries is likely to
break existing source code, it would probably be
a better idea to tag them with a special \-code
which causes a suppressible warning when seen by
assemble.c.
Logged In: YES
user_id=804543
The following is an exhaustive list of insns.dat
entries, which "trigger" this particular issue.
ADD mem,imm8 \300\1\x80\200\21 8086,SM
ADD mem,imm16 \320\300\134\1\x81\200\131 8086,SM
ADD mem,imm32 \321\300\144\1\x81\200\141 386,SM
OR mem,imm8 \300\1\x80\201\21 8086,SM
OR mem,imm16 \320\300\134\1\x81\201\131 8086,SM
OR mem,imm32 \321\300\144\1\x81\201\141 386,SM
ADC mem,imm8 \300\1\x80\202\21 8086,SM
ADC mem,imm16 \320\300\134\1\x81\202\131 8086,SM
ADC mem,imm32 \321\300\144\1\x81\202\141 386,SM
SBB mem,imm8 \300\1\x80\203\21 8086,SM
SBB mem,imm16 \320\300\134\1\x81\203\131 8086,SM
SBB mem,imm32 \321\300\144\1\x81\203\141 386,SM
AND mem,imm8 \300\1\x80\204\21 8086,SM
AND mem,imm16 \320\300\134\1\x81\204\131 8086,SM
AND mem,imm32 \321\300\144\1\x81\204\141 386,SM
SUB mem,imm8 \300\1\x80\205\21 8086,SM
SUB mem,imm16 \320\300\134\1\x81\205\131 8086,SM
SUB mem,imm32 \321\300\144\1\x81\205\141 386,SM
XOR mem,imm8 \300\1\x80\206\21 8086,SM
XOR mem,imm16 \320\300\134\1\x81\206\131 8086,SM
XOR mem,imm32 \321\300\144\1\x81\206\141 386,SM
CMP mem,imm8 \300\1\x80\207\21 8086,SM
CMP mem,imm16 \320\300\134\1\x81\207\131 8086,SM
CMP mem,imm32 \321\300\144\1\x81\207\141 386,SM
As stated before, these entries should be marked
with a new backslash code that causes a warning.
That way existing code won't break, and we won't
waste one of the precious itemplate.flags bits.
Ideally the warning should be a suppressible one.
patch from diff -Pru on nasm 0.98.39
Logged In: YES
user_id=642401
I've added a patch to fix this problem by adding an optional warning.
Logged In: YES
user_id=804543
I have reviewed the attached diff. It should be applied.