search

avrdudes / avrdude

AVRDUDE is a utility to program AVR microcontrollers
GNU General Public License v2.0
734 stars 136 forks source link

Pointer truncation in Windows 64-bit #1118

Closed mariusgreuel closed 2 years ago

mariusgreuel commented 2 years ago

I noticed there is a new MSYS2 warning, https://github.com/avrdudes/avrdude/actions/runs/3193099836/jobs/5211315290#step:5:75 which appears to be bogus as the pointer is not dereferenced, however, I spotted a pointer truncation bug:

On Windows x64, sizeof(int)==4 and sizeof(void*)==8, i.e. for pointer arithmetic/conversion, we cannot use int. Instead, we need to use size_t, ptrdiff_t, and the like.

So the line

int k;
k = buf - bufp;

needs to be changed to

ptrdiff_t k = buf - bufp;

Sure enough, the MSVC compiler warns about this problem, however, I have the relevant warnings turned off due to a lot of noise from the AVRDUDE codebase. Normally, these warnings are essential to spot these kinds of bugs.

Not sure if we should enable these warnings, and perhaps clean up the codebase. Any opinions?

I quickly ran through the warnings, and I could only find two problems in term.c:1211 and term.c:1232. For reference, here is the complete log with C4244 and C4267 turned on:

>------ Rebuild All started: Project: avrdude, Configuration: x64-Debug ------

<snip>

    [FLEX][Parser] Building scanner with win_flex 2.6.4
    [BISON][Parser] Building parser with bison 3.7.4
    Building Custom Rule D:/Work/avrdude/src/CMakeLists.txt
    arduino.c
    avr.c
D:\Work\avrdude\src\avr.c(644,18): warning C4244: '=': conversion from 'unsigned long' to 'unsigned short', possible loss of data 
D:\Work\avrdude\src\avr.c(651,18): warning C4244: '=': conversion from 'unsigned long' to 'unsigned short', possible loss of data 
D:\Work\avrdude\src\avr.c(655,13): warning C4244: '=': conversion from 'unsigned long' to 'unsigned short', possible loss of data 
    avr910.c
    avrcache.c
    avrftdi.c
D:\Work\avrdude\src\avrftdi.c(384,74): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\avrftdi.c(390,59): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\avrftdi.c(398,9): warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\avrftdi.c(432,3): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\avrftdi.c(442,72): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\avrftdi.c(453,9): warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\avrftdi.c(1049,21): warning C4244: '=': conversion from '__int64' to 'unsigned int', possible loss of data 
    avrftdi_tpi.c
    avrpart.c
D:\Work\avrdude\src\avrpart.c(429,7): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\avrpart.c(454,7): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
    bitbang.c
    buspirate.c
D:\Work\avrdude\src\buspirate.c(129,9): warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\buspirate.c(247,19): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
    butterfly.c
    config.c
    confwin.c
    crc16.c
    dfu.c
    fileio.c
D:\Work\avrdude\src\fileio.c(217,12): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\fileio.c(227,18): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(247,18): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(261,21): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(273,17): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(315,11): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\fileio.c(522,9): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\fileio.c(542,18): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(554,19): warning C4244: '=': conversion from 'unsigned __int64' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\fileio.c(568,21): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(580,17): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(610,11): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\fileio.c(1039,22): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\fileio.c(1102,12): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\fileio.c(1107,12): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\fileio.c(1149,25): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\fileio.c(1423,11): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
    flip1.c
D:\Work\avrdude\src\flip1.c(791,41): warning C4244: 'initializing': conversion from 'unsigned short' to 'unsigned char', possible loss of data 
    flip2.c
    ft245r.c
    jtagmkI.c
    jtagmkII.c
D:\Work\avrdude\src\jtagmkII.c(435,22): warning C4267: 'function': conversion from 'size_t' to 'unsigned long', possible loss of data 
D:\Work\avrdude\src\jtagmkII.c(439,22): warning C4267: 'function': conversion from 'size_t' to 'unsigned long', possible loss of data 
    Generating Code...
    Compiling...
    jtag3.c
D:\Work\avrdude\src\jtag3.c(486,40): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\jtag3.c(496,40): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\jtag3.c(466,41): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\jtag3.c(1118,31): warning C4244: 'initializing': conversion from 'double' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\jtag3.c(2223,29): warning C4244: 'initializing': conversion from 'double' to 'unsigned int', possible loss of data 
    linuxgpio.c
    linuxspi.c
    lists.c
D:\Work\avrdude\src\lists.c(447,44): warning C4267: '=': conversion from 'size_t' to 'short', possible loss of data 
    micronucleus.c
    par.c
    pgm.c
    pgm_type.c
    pickit2.c
D:\Work\avrdude\src\pickit2.c(224,36): warning C4244: '=': conversion from 'double' to 'uint8_t', possible loss of data 
    pindefs.c
    ppi.c
    ppiwin.c
    serbb_posix.c
    serbb_win32.c
    ser_avrdoper.c
D:\Work\avrdude\src\ser_avrdoper.c(260,32): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\ser_avrdoper.c(263,45): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\ser_avrdoper.c(264,58): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\ser_avrdoper.c(336,35): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\ser_avrdoper.c(320,33): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
    ser_posix.c
    ser_win32.c
D:\Work\avrdude\src\ser_win32.c(199,12): warning C4244: '=': conversion from 'SOCKET' to 'int', possible loss of data 
D:\Work\avrdude\src\ser_win32.c(389,52): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\ser_win32.c(455,33): warning C4267: 'function': conversion from 'size_t' to 'DWORD', possible loss of data 
D:\Work\avrdude\src\ser_win32.c(521,55): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\ser_win32.c(583,31): warning C4267: 'function': conversion from 'size_t' to 'DWORD', possible loss of data 
    serialupdi.c
    stk500.c
D:\Work\avrdude\src\stk500.c(1040,24): warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\stk500.c(1073,13): warning C4244: '=': conversion from 'double' to 'int', possible loss of data 
    stk500v2.c
D:\Work\avrdude\src\stk500v2.c(488,16): warning C4267: '=': conversion from 'size_t' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(548,10): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(594,10): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(1823,14): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(2078,14): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(2760,24): warning C4267: '=': conversion from 'size_t' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(2820,52): warning C4267: 'function': conversion from 'size_t' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(2987,15): warning C4244: '=': conversion from 'double' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(2988,14): warning C4244: '=': conversion from 'double' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(2996,9): warning C4244: '=': conversion from 'double' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(3013,15): warning C4244: '=': conversion from 'double' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(3869,17): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(3870,17): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(3871,12): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(3919,17): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(3920,17): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\stk500v2.c(3921,12): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
    Generating Code...
    Compiling...
    stk500generic.c
    teensy.c
    updi_link.c
D:\Work\avrdude\src\updi_link.c(128,10): warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\updi_link.c(677,41): warning C4244: 'function': conversion from 'uint16_t' to 'uint8_t', possible loss of data 
    updi_nvm.c
    updi_readwrite.c
    updi_state.c
    usbasp.c
D:\Work\avrdude\src\usbasp.c(685,25): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data 
D:\Work\avrdude\src\usbasp.c(979,36): warning C4244: '=': conversion from 'double' to 'int', possible loss of data 
D:\Work\avrdude\src\usbasp.c(987,40): warning C4244: '=': conversion from 'double' to 'int', possible loss of data 
D:\Work\avrdude\src\usbasp.c(963,21): warning C4244: 'initializing': conversion from 'double' to 'int', possible loss of data 
D:\Work\avrdude\src\usbasp.c(1266,18): warning C4244: '=': conversion from 'unsigned long' to 'uint16_t', possible loss of data 
    usb_hidapi.c
D:\Work\avrdude\src\usb_hidapi.c(249,39): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\usb_hidapi.c(239,11): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
    usb_libusb.c
D:\Work\avrdude\src\usb_libusb.c(349,40): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\usb_libusb.c(334,11): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\usb_libusb.c(433,38): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
    usbtiny.c
    update.c
    wiring.c
    xbee.c
D:\Work\avrdude\src\xbee.c(268,27): warning C4244: '+=': conversion from 'unsigned __int64' to 'long', possible loss of data 
D:\Work\avrdude\src\xbee.c(361,18): warning C4244: '=': conversion from 'time_t' to 'long', possible loss of data 
D:\Work\avrdude\src\xbee.c(509,39): warning C4244: 'initializing': conversion from '__int64' to 'const unsigned int', possible loss of data 
D:\Work\avrdude\src\xbee.c(515,40): warning C4244: 'initializing': conversion from '__int64' to 'const unsigned int', possible loss of data 
D:\Work\avrdude\src\xbee.c(962,27): warning C4267: 'function': conversion from 'size_t' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\xbee.c(1028,18): warning C4267: 'function': conversion from 'size_t' to 'unsigned int', possible loss of data 
D:\Work\avrdude\src\xbee.c(1367,32): warning C4267: 'initializing': conversion from 'size_t' to 'const unsigned char', possible loss of data 
    lexer.c
    config_gram.c
D:\Work\avrdude\out\build\x64-Debug\config_gram.y(1677,13): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
    getopt.c
    gettimeofday.c
    Generating Code...
    usleep.cpp
    libavrdude.vcxproj -> D:\Work\avrdude\out\build\x64-Debug\src\Debug\libavrdude.lib
    Building Custom Rule D:/Work/avrdude/src/CMakeLists.txt
    main.c
D:\Work\avrdude\src\main.c(218,19): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\main.c(284,17): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\main.c(494,26): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\main.c(760,11): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\main.c(772,13): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\main.c(527,20): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
    term.c
D:\Work\avrdude\src\term.c(557,34): warning C4244: 'initializing': conversion from '__int64' to 'int', possible loss of data 
D:\Work\avrdude\src\term.c(780,16): warning C4244: '=': conversion from 'unsigned long' to 'unsigned char', possible loss of data 
D:\Work\avrdude\src\term.c(1161,10): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\term.c(1184,16): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\term.c(1254,9): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
    avrintel.c
    developer_opts.c
D:\Work\avrdude\src\developer_opts.c(539,10): warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1073,21): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1135,46): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1142,42): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1145,46): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1147,44): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1149,52): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1151,54): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1154,28): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
D:\Work\avrdude\src\developer_opts.c(1156,28): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
    whereami.c
    Generating Code...
       Creating library D:/Work/avrdude/out/build/x64-Debug/src/Debug/avrdude.lib and object D:/Work/avrdude/out/build/x64-Debug/src/Debug/avrdude.exp
    avrdude.vcxproj -> D:\Work\avrdude\out\build\x64-Debug\src\Debug\avrdude.exe
    Building Custom Rule D:/Work/avrdude/CMakeLists.txt

Rebuild All succeeded.
stefanrueger commented 2 years ago

clean up the codebase. Any opinions?

Should do, though many of these are not real issues. Particularly the size_t vs int ones. Often, when I change a particular file, I would run a -Wall -Wpedantic type of compilation and then, en passant address those issues the compiler considers not clean. Just so the noise abates at higher warning levels. The risk, of course, is to introduce a bug when doing so (vvv easy espec with signed/unsigned issues). So, I always try to be careful about that kind of thing.