|
From: Florian K. <fl...@ei...> - 2013-09-30 13:21:48
|
Hi,
here's the fallout from a BEAM run earlier today. Four new things:
"coregrind/m_oset.c", line 931: Statement is unreachable
COMMENT: This one was found by the EDG frontend. It produced loads of
messages so I did not spot this in earlier runs.
Not sure.. perhaps this code was meant to be reached with a
missing break somewhere?
-- ERROR23(heap_memory) /*memory leak*/
"auxprogs/valgrind-di-server.c", line 783: memory leak
ONE POSSIBLE PATH LEADING TO THE ERROR:
"auxprogs/valgrind-di-server.c", line 690: allocating using `calloc'
(this memory will not be freed)
"auxprogs/valgrind-di-server.c", line 690: assigning into `req'
"auxprogs/valgrind-di-server.c", line 692: allocating using `calloc'
"auxprogs/valgrind-di-server.c", line 692: assigning into `req->data'
"auxprogs/valgrind-di-server.c", line 783: assigning into `req' (losing
last pointer to the memory)
COMMENT: req->data leaks.
-- ERROR23(heap_memory) /*memory leak*/
"coregrind/launcher-linux.c", line 112: memory leak
ONE POSSIBLE PATH LEADING TO THE ERROR:
"coregrind/launcher-linux.c", line 87: allocating using `malloc' (this
memory will not be freed)
"coregrind/launcher-linux.c", line 87: assigning into `fullname'
"coregrind/launcher-linux.c", line 88: the if-condition is false
"coregrind/launcher-linux.c", line 91: loop entry condition is false,
therefore exiting from the loop started on line 91
"coregrind/launcher-linux.c", line 112: returning `clientname'
"coregrind/launcher-linux.c", line 112: deallocating `fullname' because
exiting its scope (losing last pointer to the memory)
COMMENT: I'll fix this one.
-- ERROR2 /*operating on NULL*/
"coregrind/m_syswrap/syswrap-generic.c", line 3971: invalid operation
involving NULL pointer
ONE POSSIBLE PATH LEADING TO THE ERROR:
"coregrind/m_syswrap/syswrap-generic.c", line 3966: conjunct is false
(used as evidence that error is possible)
"coregrind/m_syswrap/syswrap-generic.c", line 3966: the if-condition is
false
"coregrind/m_syswrap/syswrap-generic.c", line 3970: the if-condition is
true
"coregrind/m_syswrap/syswrap-generic.c", line 3971: using operation
`->' to dereference NULL pointer `(struct vki_rlimit *)ARG2'
COMMENT: Line 3966 considers the case that ARG2 could be NULL, line 3971
dereferences ARG2.
Perhaps something like this is needed:
if (ARG2 &&
(((struct vki_rlimit *)ARG2)->rlim_cur > VG_(fd_hard_limit)
|| ((struct vki_rlimit *)ARG2)->rlim_max !=VG_(fd_hard_limit))) {
The same thing also happens on lines 3981 and 3991.
|
|
From: Philippe W. <phi...@sk...> - 2013-09-30 19:46:22
|
On Mon, 2013-09-30 at 15:21 +0200, Florian Krohm wrote:
> Hi,
>
> here's the fallout from a BEAM run earlier today. Four new things:
>
>
> "coregrind/m_oset.c", line 931: Statement is unreachable
>
> COMMENT: This one was found by the EDG frontend. It produced loads of
> messages so I did not spot this in earlier runs.
> Not sure.. perhaps this code was meant to be reached with a
> missing break somewhere?
This I do not understand.
On the last svn trunk, line 931 is there:
Word VG_(OSetGen_Size)(const AvlTree* t)
{
vg_assert(t);
return t->nElems;
}
<<<<<<<<<<<<<<<<<<<<<<<<<<< line 931>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Word VG_(OSetWord_Size)(AvlTree* t)
{
return VG_(OSetGen_Size)(t);
}
> COMMENT: Line 3966 considers the case that ARG2 could be NULL, line 3971
> dereferences ARG2.
> Perhaps something like this is needed:
>
> if (ARG2 &&
> (((struct vki_rlimit *)ARG2)->rlim_cur > VG_(fd_hard_limit)
> || ((struct vki_rlimit *)ARG2)->rlim_max !=VG_(fd_hard_limit))) {
>
> The same thing also happens on lines 3981 and 3991.
Or maybe at the beginning do:
if (!ARG2)
SET_STATUS_Failure( VKI_EFAULT);
else if ....
This will ensure the rest should be able to dereference ARG2.
(there are already a bunch of places doing such checks, sometimes
also with calls to ML_(valid_client_addr) or
VG_(am_is_valid_for_client).
Philippe
|
|
From: Florian K. <fl...@ei...> - 2013-09-30 20:25:46
|
On 09/30/2013 09:46 PM, Philippe Waroquiers wrote: >> "coregrind/m_oset.c", line 931: Statement is unreachable >> >> COMMENT: This one was found by the EDG frontend. It produced loads of >> messages so I did not spot this in earlier runs. >> Not sure.. perhaps this code was meant to be reached with a >> missing break somewhere? > This I do not understand. > On the last svn trunk, line 931 is there: Sorry, my mistake. Should have said: line 913, not line 931. Florian |
|
From: Philippe W. <phi...@sk...> - 2013-09-30 21:45:31
|
On Mon, 2013-09-30 at 22:25 +0200, Florian Krohm wrote: > On 09/30/2013 09:46 PM, Philippe Waroquiers wrote: > > >> "coregrind/m_oset.c", line 931: Statement is unreachable > >> > >> COMMENT: This one was found by the EDG frontend. It produced loads of > >> messages so I did not spot this in earlier runs. > >> Not sure.. perhaps this code was meant to be reached with a > >> missing break somewhere? > > This I do not understand. > > On the last svn trunk, line 931 is there: > > Sorry, my mistake. Should have said: line 913, not line 931. Took a quick look at it, the dead code looks strange. There is no test of VG_(OSetGen_ResetIterAt) in unit_oset.c. I will take a look at adding a test for VG_(OSetGen_ResetIterAt). Maybe that will show a bug in this area. Philippe |
|
From: Julian S. <js...@ac...> - 2013-10-14 10:09:49
|
On 09/30/2013 03:21 PM, Florian Krohm wrote:
> here's the fallout from a BEAM run earlier today. Four new things:
Thanks for those. It continues to pick up interesting stuff.
> -- ERROR23(heap_memory) /*memory leak*/
> "auxprogs/valgrind-di-server.c", line 783: memory leak
> ONE POSSIBLE PATH LEADING TO THE ERROR:
> "auxprogs/valgrind-di-server.c", line 690: allocating using `calloc'
> (this memory will not be freed)
> "auxprogs/valgrind-di-server.c", line 690: assigning into `req'
> "auxprogs/valgrind-di-server.c", line 692: allocating using `calloc'
> "auxprogs/valgrind-di-server.c", line 692: assigning into `req->data'
> "auxprogs/valgrind-di-server.c", line 783: assigning into `req' (losing
> last pointer to the memory)
>
> COMMENT: req->data leaks.
Interesting. This is a debuginfo server handling a request frame
(req) and producing a response frame (res). By comparing the line
with the three ifs that follow it, it seems to me that the assignment
should be to res and not to req. The leak occurs because it wrongly
assigns to req and hence leaks the request frame. It only doesn't
crash the server because this is an error path which I think has
never been tested. +1 for static analysis!
> -- ERROR2 /*operating on NULL*/
> "coregrind/m_syswrap/syswrap-generic.c", line 3971: invalid operation
> involving NULL pointer
> ONE POSSIBLE PATH LEADING TO THE ERROR:
> "coregrind/m_syswrap/syswrap-generic.c", line 3966: conjunct is false
> (used as evidence that error is possible)
> "coregrind/m_syswrap/syswrap-generic.c", line 3966: the if-condition is
> false
> "coregrind/m_syswrap/syswrap-generic.c", line 3970: the if-condition is
> true
> "coregrind/m_syswrap/syswrap-generic.c", line 3971: using operation
> `->' to dereference NULL pointer `(struct vki_rlimit *)ARG2'
>
> COMMENT: Line 3966 considers the case that ARG2 could be NULL, line 3971
> dereferences ARG2.
> Perhaps something like this is needed:
>
> if (ARG2 &&
> (((struct vki_rlimit *)ARG2)->rlim_cur > VG_(fd_hard_limit)
> || ((struct vki_rlimit *)ARG2)->rlim_max !=VG_(fd_hard_limit))) {
ARG2 is dereferenced both in the condition and the else-clause, though,
so I think the extra check needs to be moved one level outwards, hence:
else if (ARG2 && arg1 == VKI_RLIMIT_NOFILE) {
else if (ARG2 && arg1 == VKI_RLIMIT_DATA) {
else if (ARG2 && arg1 == VKI_RLIMIT_STACK && tid == 1) {
J
|