There's some obvious bugs below in a bunch of packages. The 2nd and 3rd arguments to memset calls are the wrong way around. I found these after grepping through a make prep'd devel/ tree.
15 hits out of 100G of source code isn't that bad, but we can do better!
Dave
Checking ./afflib/afflib-3.5.2/lib/s3_glue.cpp Found memset with swapped arguments. 303: memset(b64str,sizeof(b64str),0);
Checking ./afflib/afflib-3.5.2/lib/vnode_s3.cpp Found memset with swapped arguments. 205: memset(segname,segname_len,0);
Checking ./afflib/afflib-3.5.2/lib/crypto.cpp Found memset with swapped arguments. 975: memset(decrypted,total_encrypted_bytes,0); // overwrite our temp buffer
Checking ./panoglview/panoglview-0.2.2/src/panocanvas.cpp Found memset with swapped arguments. 160: memset(tmp,m_maxsize*m_maxsize,0);
Checking ./condor/condor-7.2.4/src/condor_c++_util/read_user_log_state.cpp Found memset with swapped arguments. 497: memset( istate->m_base_path, sizeof(istate->m_base_path), 0 );
Checking ./milkytracker/milkytracker-0.90.80/src/milkyplay/LoaderPSM.cpp Found memset with swapped arguments. 999: memset(packed,size-4+5,0);
Checking ./sim/sim/sim/sockfactory.cpp Found memset with swapped arguments. 546: memset(&addr, sizeof(addr), 0);
Checking ./commoncpp2/commoncpp2-1.7.3/src/thread.cpp Found memset with swapped arguments. 525: memset(&act, sizeof(act), 0);
Checking ./commoncpp2/commoncpp2-1.7.3/src/socket.cpp Found memset with swapped arguments. 1571: memset(&group, sizeof(group), 0);
Checking ./celestia/celestia-1.5.1/src/celestia/winmain.cpp Found memset with swapped arguments. 2181: memset(&info, sizeof(info), 0);
Checking ./scummvm/scummvm-0.13.1/engines/tinsel/scene.cpp Found memset with swapped arguments. 132: memset(&tempStruc, sizeof(SCENE_STRUC), 0);
Checking ./aqsis/aqsis-1.6.0/tools/displays/sdcWin32/d_sdcWin32.cpp Found memset with swapped arguments. 250: memset(&g_Data, sizeof(AppData), 0);
Checking ./aqsis/aqsis-1.6.0/tools/displays/sdcBMP/d_sdcBMP.cpp Found memset with swapped arguments. 172: memset(&g_Data, sizeof(AppData), 0);
Checking ./arm4/arm4-0.8.2/src/libarm4db/berkeleydb/BerkeleyDB_report.cpp Found memset with swapped arguments. 1603: memset (summary_ptr, sizeof (*summary_ptr), 0);
Checking ./arm4/arm4-0.8.2/src/libarm4db/Arm4dbDaemonSharedMemory.cpp Found memset with swapped arguments. 558: memset (stats_ptr, sizeof (*stats_ptr), 0);
On Wed, Nov 25, 2009 at 01:43:13PM -0500, Dave Jones wrote:
There's some obvious bugs below in a bunch of packages. The 2nd and 3rd arguments to memset calls are the wrong way around. I found these after grepping through a make prep'd devel/ tree.
15 hits out of 100G of source code isn't that bad, but we can do better!
glibc headers warn about this (when -D_FORTIFY_SOURCE=2), so a faster way would be just grep through all packages' build.log files (preferrably on the box where they are stored to avoid downloading it all).
Jakub
On Wed, Nov 25, 2009 at 01:58:38PM -0500, Jakub Jelinek wrote:
On Wed, Nov 25, 2009 at 01:43:13PM -0500, Dave Jones wrote:
There's some obvious bugs below in a bunch of packages. The 2nd and 3rd arguments to memset calls are the wrong way around. I found these after grepping through a make prep'd devel/ tree.
15 hits out of 100G of source code isn't that bad, but we can do better!
glibc headers warn about this (when -D_FORTIFY_SOURCE=2), so a faster way would be just grep through all packages' build.log files (preferrably on the box where they are stored to avoid downloading it all).
Can we make it fail the build instead of warning ? A zero sized memset is always a bug.
Dave
On 11/25/2009 02:03 PM, Dave Jones wrote:
On Wed, Nov 25, 2009 at 01:58:38PM -0500, Jakub Jelinek wrote:
glibc headers warn about this (when -D_FORTIFY_SOURCE=2), so a faster way would be just grep through all packages' build.log files (preferrably on the box where they are stored to avoid downloading it all).
Can we make it fail the build instead of warning ? A zero sized memset is always a bug.
No, memset(,,0) is not always a bug. A null region is not automatically a bug. Here is one example:
struct Foo { long x; char hole[8 - sizeof(long)]; } foo;
memset(&foo.hole, 0, sizeof(foo.hole));
On a LP64-bit machine such as x86_64, this is memset(&foo.hole, 0, 0), which is *NOT* a bug.
Perhaps the best that can be expected is for the compiler to warn if _builtin_memset has a third argument which is known to be a compile-time constant zero. But such a warning must be optional, for there are legitimate use cases. Also, if the second argument to _builtin_memset is a compile-time constant which cannot be represented in one byte (considering both signed and unsigned cases) then another optional warning may be appropriate.
John Reiser jreiser@bitwagon.com writes:
On 11/25/2009 02:03 PM, Dave Jones wrote:
A zero sized memset is always a bug.
No, memset(,,0) is not always a bug.
I think it's reasonably safe to assume that a *literal constant* zero in the third argument is a bug. Whether the header macros can distinguish that from compile-time-constant expressions is an interesting question, but if they can, +1 for throwing an error.
regards, tom lane
On Wed, 2009-11-25 at 23:39 -0500, Tom Lane wrote:
John Reiser jreiser@bitwagon.com writes:
On 11/25/2009 02:03 PM, Dave Jones wrote:
A zero sized memset is always a bug.
No, memset(,,0) is not always a bug.
I think it's reasonably safe to assume that a *literal constant* zero in the third argument is a bug. Whether the header macros can distinguish that from compile-time-constant expressions is an interesting question, but if they can, +1 for throwing an error.
I logged some time ago a trivial patch of https://bugzilla.redhat.com/show_bug.cgi?id=532492 which is apparently in rawhide now to generally avoid a warning on memset(foo, 0, 0)
C.
Hi All, Iam new to this fedora world.. a small question on the below discussion:
It is mentioned that having, zero in the third argument is legitimate use cases. Can somebody direct me to such a use case, as i feel, giving memset a zero, is asking it, not to do anything [ might have side effects, not sure from my end, though ].
Regards Roopesh M.
On Thu, Nov 26, 2009 at 2:24 PM, Caolán McNamara caolanm@redhat.com wrote:
On Wed, 2009-11-25 at 23:39 -0500, Tom Lane wrote:
John Reiser jreiser@bitwagon.com writes:
On 11/25/2009 02:03 PM, Dave Jones wrote:
A zero sized memset is always a bug.
No, memset(,,0) is not always a bug.
I think it's reasonably safe to assume that a *literal constant* zero in the third argument is a bug. Whether the header macros can distinguish that from compile-time-constant expressions is an interesting question, but if they can, +1 for throwing an error.
I logged some time ago a trivial patch of https://bugzilla.redhat.com/show_bug.cgi?id=532492 which is apparently in rawhide now to generally avoid a warning on memset(foo, 0, 0)
C.
-- fedora-devel-list mailing list fedora-devel-list@redhat.com https://www.redhat.com/mailman/listinfo/fedora-devel-list
On Fri, Nov 27, 2009 at 1:26 AM, Roopesh Majeti roopesh.majeti@gmail.com wrote:
Hi All, Iam new to this fedora world.. a small question on the below discussion: It is mentioned that having, zero in the third argument is legitimate use cases. Can somebody direct me to such a use case, as i feel, giving memset a zero, is asking it, not to do anything [ might have side effects, not sure from my end, though ].
It's legitimate because the zero may ultimately be derived from macro values and restructuring the code to avoid the memset depending on defined values may be non-trivial or even impossible.
John Reiser provided a good example: http://www.linux-archive.org/fedora-development/286221-memset-bugs.html
Where its not a programming bug the memset(,,0) is harmless: GCC optimizes it out completely.
A literal zero prior to preprocessing is either a bug, or some kind of dead- code causing place-holder.
On Fri, Nov 27, 2009 at 03:28:19AM -0500, Gregory Maxwell wrote:
A literal zero prior to preprocessing is either a bug, or some kind of dead- code causing place-holder.
Not necessarily .. the C code itself may be generated from something else.
Rich.
On 11/27/2009 06:03 AM, Richard W.M. Jones wrote:
On Fri, Nov 27, 2009 at 03:28:19AM -0500, Gregory Maxwell wrote:
A literal zero prior to preprocessing is either a bug, or some kind of dead- code causing place-holder.
Not necessarily .. the C code itself may be generated from something else.
Rich.
In which case the C code is no longer "source" and should be excluded from the analysis.
--CJD
On 11/27/2009 02:25 PM, Casey Dahlin wrote:
On 11/27/2009 06:03 AM, Richard W.M. Jones wrote:
On Fri, Nov 27, 2009 at 03:28:19AM -0500, Gregory Maxwell wrote:
A literal zero prior to preprocessing is either a bug, or some kind of dead- code causing place-holder.
Not necessarily .. the C code itself may be generated from something else.
Rich.
In which case the C code is no longer "source" and should be excluded from the analysis.
No, when swig (or whatever) produces bad code, we still want the compiler to identify it and toss it. It's then up to the packager to realize it's swig producing the bad code, but it isn't magically good code that doesn't result in real bugs.
On 11/30/2009 10:39 AM, Peter Jones wrote:
On 11/27/2009 02:25 PM, Casey Dahlin wrote:
On 11/27/2009 06:03 AM, Richard W.M. Jones wrote:
On Fri, Nov 27, 2009 at 03:28:19AM -0500, Gregory Maxwell wrote:
A literal zero prior to preprocessing is either a bug, or some kind of dead- code causing place-holder.
Not necessarily .. the C code itself may be generated from something else.
Rich.
In which case the C code is no longer "source" and should be excluded from the analysis.
No, when swig (or whatever) produces bad code, we still want the compiler to identify it and toss it. It's then up to the packager to realize it's swig producing the bad code, but it isn't magically good code that doesn't result in real bugs.
The compiler isn't doing these checks, but point taken.
On a tangent, what of these checks if any should be put into the compiler? Compile-time bounds checking of library functions is kind of "magical" and un-C-like, but its not unprecedented (printf argument checking for example).
--CJD
On 11/30/2009 11:39 AM, Casey Dahlin wrote:
On 11/30/2009 10:39 AM, Peter Jones wrote:
On 11/27/2009 02:25 PM, Casey Dahlin wrote:
On 11/27/2009 06:03 AM, Richard W.M. Jones wrote:
On Fri, Nov 27, 2009 at 03:28:19AM -0500, Gregory Maxwell wrote:
A literal zero prior to preprocessing is either a bug, or some kind of dead- code causing place-holder.
Not necessarily .. the C code itself may be generated from something else.
Rich.
In which case the C code is no longer "source" and should be excluded from the analysis.
No, when swig (or whatever) produces bad code, we still want the compiler to identify it and toss it. It's then up to the packager to realize it's swig producing the bad code, but it isn't magically good code that doesn't result in real bugs.
The compiler isn't doing these checks, but point taken.
Go read Jakub's reply again ;)
https://www.redhat.com/archives/fedora-devel-list/2009-November/msg01966.htm...
On 11/30/2009 01:10 PM, Peter Jones wrote:
On 11/30/2009 11:39 AM, Casey Dahlin wrote:
On 11/30/2009 10:39 AM, Peter Jones wrote:
On 11/27/2009 02:25 PM, Casey Dahlin wrote:
On 11/27/2009 06:03 AM, Richard W.M. Jones wrote:
On Fri, Nov 27, 2009 at 03:28:19AM -0500, Gregory Maxwell wrote:
A literal zero prior to preprocessing is either a bug, or some kind of dead- code causing place-holder.
Not necessarily .. the C code itself may be generated from something else.
Rich.
In which case the C code is no longer "source" and should be excluded from the analysis.
No, when swig (or whatever) produces bad code, we still want the compiler to identify it and toss it. It's then up to the packager to realize it's swig producing the bad code, but it isn't magically good code that doesn't result in real bugs.
The compiler isn't doing these checks, but point taken.
Go read Jakub's reply again ;)
https://www.redhat.com/archives/fedora-devel-list/2009-November/msg01966.htm...
I stand corrected.
--CJD
devel@lists.stg.fedoraproject.org
-
Caolán McNamara -
Casey Dahlin -
Dave Jones -
Gregory Maxwell -
Jakub Jelinek -
John Reiser -
Peter Jones -
Richard W.M. Jones -
roopesh majeti -
Tom Lane