Hi,
a few days ago I found a bug in ole32/compobj.c where we did something like:
if ( foo & FLAG) .... where FLAG=0
This 'inspired' me to check for more of these kind of checks.
One that I already found is in dlls/dplayx/dplay.c
We are checking for DPSET_REMOTE (which is zero) where we should have checked for !DPSET_LOCAL or something alike.
Is it worthwhile to set up a Janitorial task for this or are these plain bugs? On the one hand it will be hard to find these and you have to know the code of course.
Cheers,
Paul.
On Wed, Jan 12, 2005 at 09:04:20PM +0100, Paul Vriens wrote:
Is it worthwhile to set up a Janitorial task for this or are these plain bugs? On the one hand it will be hard to find these and you have to know the code of course.
Yeah, I think it's worthwhile to look at such cases, but I don't think we should have a janitorial task for them: they are far and few in between, hard to find, etc.
On Wed, 12 Jan 2005, Dimitrie O. Paun wrote:
On Wed, Jan 12, 2005 at 09:04:20PM +0100, Paul Vriens wrote:
Is it worthwhile to set up a Janitorial task for this or are these plain bugs? On the one hand it will be hard to find these and you have to know the code of course.
Yeah, I think it's worthwhile to look at such cases, but I don't think we should have a janitorial task for them: they are far and few in between, hard to find, etc.
Actually it's not that hard. You can even do it with a one-line bash script:
regexp=`sed -e 's/^ *# *define *([a-zA-Z_][a-zA-Z0-9_]*) *0 *$/\1/' -e 't' -e 's/^ *# *define *([a-zA-Z_][a-zA-Z0-9_]*) *0x00*L* *$/\1/' -e 't' -e 'd' include/*.h`;regexp=`echo $regexp | sed -e 's/ NULL / /' -e 's/ /|/g'`;egrep -r -n "& *($regexp)[^a-zA-Z0-9_]" .
Ok, it's a long line but it's just one line anyway<g>. Here's what I get:
./dlls/comctl32/listview.c:6297: if (nColumn == 0 || lpColumn->fmt & LVCFMT_LEFT) ./dlls/comctl32/pager.c:844: if (!(dwStyle & PGS_HORZ) && !(dwStyle & PGS_VERT)) ./dlls/commdlg/filedlgbrowser.c:320: (wFlags & SBSP_ABSOLUTE) ? "SBSP_ABSOLUTE" : "SBPS_????"); ./dlls/dplayx/dplay.c:1164: if( ( dwFlags & DPSET_REMOTE ) && ./dlls/dplayx/dplay.c:1180: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:1363: if( ( dwFlags & DPSET_REMOTE ) && ./dlls/dplayx/dplay.c:1379: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:2430: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:2635: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3061: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3074: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3159: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3169: if( dwFlags & DPSET_REMOTE ) ./dlls/winmm/mciseq/mcimidi.c:1359: if (lpParms->dwAudio & MCI_SET_AUDIO_ALL) ./dlls/winmm/mciwave/mciwave.c:1233: if (lpParms->dwAudio & MCI_SET_AUDIO_ALL)
False positives: ./dlls/psapi/tests/module.c:95: ok((retval && ERROR_SUCCESS == GetLastError()) || ./dlls/psapi/tests/module.c:119: ok( (retval && ERROR_SUCCESS == GetLastError()) ||
regexp=`sed -e 's/^ *# *define *([a-zA-Z_][a-zA-Z0-9_]*) *0 *$/\1/' -e 't' -e 's/^ *# *define *([a-zA-Z_][a-zA-Z0-9_]*) *0x00*L* *$/\1/' -e 't' -e 'd' include/*.h`;regexp=`echo $regexp | sed -e 's/ NULL / /' -e 's/ /|/g'`;egrep -r -n "& *($regexp)[^a-zA-Z0-9_]" .
Francois Gouget fgouget@free.fr http://fgouget.free.fr/ Linux: It is now safe to turn on your computer.
Hi Francois,
how do you cater for the following in your one-liner:
include/accctrl.h:#define ACTRL_RESERVED 0x00000000 include/accctrl.h:#define ACTRL_DS_OPEN ACTRL_RESERVED
good luck :-)
But seriously what would be the best way to fix up dplayx.c?
Cheers,
Paul.
On Thu, 13 Jan 2005, Paul Vriens wrote: [...]
how do you cater for the following in your one-liner:
include/accctrl.h:#define ACTRL_RESERVED 0x00000000 include/accctrl.h:#define ACTRL_DS_OPEN ACTRL_RESERVED
good luck :-)
The previous script doesn't. Handling this is a bit more tricky, mostly because of sed's restrictions on regular expressions.
Fortunately perl is there to save the day! There you go:
new=`perl -n -e 'chomp;if (s/^ *# *define *([a-zA-Z_][a-zA-Z0-9_]*) *0(x0+L?)? *$/$1/) {print;print " ";}' include/*.h` while [ -n "$new" ] do new=`echo "$new" | sed -e 's/ $//' -e 's/ NULL / /' -e 's/ /|/g'` if [ -z "$regexp" ] then regexp="$new" else regexp="$regexp|$new" fi new=`perl -n -e "chomp;if (s/^ *# *define *([a-zA-Z_][a-zA-Z0-9_]*) *($new) *\$/\$1/) {print;print ' ';}" include/*.h` echo "Synonyms: $new" done find . -name "*.[chly]" -print0 | xargs -0 perl -n -e "if (eof) {close ARGV;};if (/& *($regexp)(?![a-zA-Z0-9_]| ==)/ or /($regexp) *&[^&]/) {print "$ARGV:$.: $_";}"
Hopefully it won't be too line wrapped. And here's the result, with no false positives this time, thanks to perl's negative lookhead.
./dlls/comctl32/listview.c:6297: if (nColumn == 0 || lpColumn->fmt & LVCFMT_LEFT) ./dlls/comctl32/pager.c:844: if (!(dwStyle & PGS_HORZ) && !(dwStyle & PGS_VERT)) ./dlls/commdlg/filedlgbrowser.c:320: (wFlags & SBSP_ABSOLUTE) ? "SBSP_ABSOLUTE" : "SBPS_????"); ./dlls/dplayx/dplay.c:1164: if( ( dwFlags & DPSET_REMOTE ) && ./dlls/dplayx/dplay.c:1180: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:1363: if( ( dwFlags & DPSET_REMOTE ) && ./dlls/dplayx/dplay.c:1379: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:2430: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:2635: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3061: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3074: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3159: if( dwFlags & DPSET_REMOTE ) ./dlls/dplayx/dplay.c:3169: if( dwFlags & DPSET_REMOTE ) ./dlls/winmm/mciseq/mcimidi.c:1359: if (lpParms->dwAudio & MCI_SET_AUDIO_ALL) ./dlls/winmm/mciwave/mciwave.c:1233: if (lpParms->dwAudio & MCI_SET_AUDIO_ALL)
But seriously what would be the best way to fix up dplayx.c?
Not sure, maybe:
if (dwFlags == DPSET_REMOTE)
or
if (!dwFlags)
but I find the latter a bit less clear.
On Thu, Jan 13, 2005 at 04:14:36PM +0100, Francois Gouget wrote:
The previous script doesn't. Handling this is a bit more tricky, mostly because of sed's restrictions on regular expressions.
Heh, nice script. Maybe we should have such an item on the page after all, maybe together with the script?
-
Dimitrie O. Paun -
Francois Gouget -
Paul Vriens