| Message ID | AANLkTin5nuHB6181GJOZJB9LqyS6uGpMzwxscZYRpM=L@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Blue Swirl <blauwirbel@gmail.com> writes: > Compiling with GCC 4.6.0 20100925 produced warnings: > /src/qemu/hw/eepro100.c: In function 'eepro100_read4': > /src/qemu/hw/eepro100.c:1351:14: error: 'val' may be used > uninitialized in this function [-Werror=uninitialized] > /src/qemu/hw/eepro100.c: In function 'eepro100_read2': > /src/qemu/hw/eepro100.c:1328:14: error: 'val' may be used > uninitialized in this function [-Werror=uninitialized] > /src/qemu/hw/eepro100.c: In function 'eepro100_read1': > /src/qemu/hw/eepro100.c:1285:13: error: 'val' may be used > uninitialized in this function [-Werror=uninitialized] > > Fix by initializing 'val' at start. I'm worried this sweeps bugs under the carpet. When addr is out of bounds, these function return garbage. Your patch makes them return 0 instead. Can that happen? Shouldn't we catch and flag it?
On Thu, Oct 7, 2010 at 9:31 AM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> Compiling with GCC 4.6.0 20100925 produced warnings: >> /src/qemu/hw/eepro100.c: In function 'eepro100_read4': >> /src/qemu/hw/eepro100.c:1351:14: error: 'val' may be used >> uninitialized in this function [-Werror=uninitialized] >> /src/qemu/hw/eepro100.c: In function 'eepro100_read2': >> /src/qemu/hw/eepro100.c:1328:14: error: 'val' may be used >> uninitialized in this function [-Werror=uninitialized] >> /src/qemu/hw/eepro100.c: In function 'eepro100_read1': >> /src/qemu/hw/eepro100.c:1285:13: error: 'val' may be used >> uninitialized in this function [-Werror=uninitialized] >> >> Fix by initializing 'val' at start. > > I'm worried this sweeps bugs under the carpet. > > When addr is out of bounds, these function return garbage. Your patch > makes them return 0 instead. Can that happen? Shouldn't we catch and > flag it? 0 is a nice default value. Adding an assert is not OK, guest shouldn't be able to terminate QEMU. Adding a check which spams stdio is not so nice either. I think it's even impossible with current QEMU for addr to be out of bounds, the area is registered with size PCI_MEM_SIZE.
Am 07.10.2010 11:31, schrieb Markus Armbruster: > Blue Swirl <blauwirbel@gmail.com> writes: > >> Compiling with GCC 4.6.0 20100925 produced warnings: >> /src/qemu/hw/eepro100.c: In function 'eepro100_read4': >> /src/qemu/hw/eepro100.c:1351:14: error: 'val' may be used >> uninitialized in this function [-Werror=uninitialized] >> /src/qemu/hw/eepro100.c: In function 'eepro100_read2': >> /src/qemu/hw/eepro100.c:1328:14: error: 'val' may be used >> uninitialized in this function [-Werror=uninitialized] >> /src/qemu/hw/eepro100.c: In function 'eepro100_read1': >> /src/qemu/hw/eepro100.c:1285:13: error: 'val' may be used >> uninitialized in this function [-Werror=uninitialized] >> >> Fix by initializing 'val' at start. > > I'm worried this sweeps bugs under the carpet. > > When addr is out of bounds, these function return garbage. Your patch > makes them return 0 instead. Can that happen? Shouldn't we catch and > flag it? We should. I'll test new code which uses an assertion instead of the if statements, so a new patch might be ready until end of next week. Blue Swirl's patch does no harm, so it could be applied nevertheless if compiler warnings should be fixed now (I had the same kind of patch in my queue). Stefan
diff --git a/hw/eepro100.c b/hw/eepro100.c index 2b75c8f..adc579f 100644 --- a/hw/eepro100.c +++ b/hw/eepro100.c @@ -1282,7 +1282,7 @@ static void eepro100_write_port(EEPRO100State * s, uint32_t val) static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr) { - uint8_t val; + uint8_t val = 0; if (addr <= sizeof(s->mem) - sizeof(val)) { memcpy(&val, &s->mem[addr], sizeof(val));
Compiling with GCC 4.6.0 20100925 produced warnings: /src/qemu/hw/eepro100.c: In function 'eepro100_read4': /src/qemu/hw/eepro100.c:1351:14: error: 'val' may be used uninitialized in this function [-Werror=uninitialized] /src/qemu/hw/eepro100.c: In function 'eepro100_read2': /src/qemu/hw/eepro100.c:1328:14: error: 'val' may be used uninitialized in this function [-Werror=uninitialized] /src/qemu/hw/eepro100.c: In function 'eepro100_read1': /src/qemu/hw/eepro100.c:1285:13: error: 'val' may be used uninitialized in this function [-Werror=uninitialized] Fix by initializing 'val' at start. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/eepro100.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) } @@ -1325,7 +1325,7 @@ static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr) static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr) { - uint16_t val; + uint16_t val = 0; if (addr <= sizeof(s->mem) - sizeof(val)) { memcpy(&val, &s->mem[addr], sizeof(val)); } @@ -1348,7 +1348,7 @@ static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr) static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr) { - uint32_t val; + uint32_t val = 0; if (addr <= sizeof(s->mem) - sizeof(val)) { memcpy(&val, &s->mem[addr], sizeof(val)); }