patchConcurrent Versions System - Patches: patch #5911, CVS 1.12.13 - fixed memory leaks...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #5911: CVS 1.12.13 - fixed memory leaks errors reported by valgrind/memcheck

Submitter:  None
Submitted:  Wed 02 May 2007 02:51:05 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  mdb Originator Email:  -email is unavailable-
Open/Closed:  Closed Fixed Release:  None
Fixed Feature Release:  1.12.14

Wed 06 Jun 2007 11:39:10 AM UTC, comment #4: 

regarding calls to getgrnam():
'man getgrnam' on my Linux PC (Intel-based, Linux kernel 2.6.18.3) states:
'The return value may point to static area'.
since valgrind shows many leaks coming from this function call in my environment, apparently it is not doing static allocation in this environment.
I think that finding in the configure script how it works is possible but not easy to implement, so it's not worth it.

memset() after xnrealloc in history.c is wrong indeed.
however,  zeroing still needs to be done on newly allocated memory for hrecs, so I need to rewrite that bit to zero just the new elements of the array. so the correct form should be:

memset (hrec_head + (hrec_max - HREC_INCREMENT) sizeof(struct hrec), 0, HREC_INCREMENT sizeof(struct hrec));

attached is yet another patch file that fixes this.


(file #12966)

Yaron Yogev <yyogev>
Mon 04 Jun 2007 08:48:35 AM UTC, comment #3: 

important:
main.c - free_config() call in line 1190 (in my patch) should only be done if (!server_active)
(caused hang on exit due to corrupted memory, in pserver mode)

This becomes irrelevant in version 1.270 of main.c, committed by mdb on 30-May-07. However, in the case anyone wanted just my fixes, I am attaching a tiny patch to this problem.



(file #12944)

Yaron Yogev <yyogev>
Wed 30 May 2007 11:22:29 PM UTC, comment #2: 

I added a --valgrind option to sanity.sh to allow more leaks
to be found. I tested with both valgrind 3.2.3 and
valgrind 2.2.0. There are still some reported memory leaks,
but I didn't try to track them all down. Many of them are
not real as the memory is cleaned up by various of the
cleanup functions on exit.

(file #12906)

Mark D. Baushke <mdb>
Group administrator
Sat 26 May 2007 07:04:50 PM UTC, comment #1: 

The patch to add.c is okay, but would be more efficient if put
before the 'goto skip_this_file;' statement where it is certain
that vers is not NULL.

The patch to admin.c is wrong as getgrnam() returns a pointer
to an internal static object and that pointer should NOT be
free()d. This code was also moved into subr.c::is_admin() in
the main trunk

The patches to checkout.c, diff.c, find_names.c, import.c, ls.c
all look good (modulo formatting in a few cases).

The edit.c patches to find_editors_and_output() and edit_fileproc()
were still good. The others had already been fixed.

A comment was desirable in hash.c::mergelists() possibly
something like:
        /* If the node is already in the list, then free
           the duplicate which was not inserted. */

In history.c, there is no need to check fname for non-NULL before
doing the free() on it. Also, the memset() in read_hrecs_file()
after the xnrealloc() call seems wrong to me.

Why are you zeroing out hrec_max bytes?
I would have thought you would want to zero the records that
were added to the overall structure (i.e., the last
HREC_INCREMENT * sizeof (struct href) bytes) if they needed
to be initialized at all.

I still have to go through your patches to main.c,
parseinfo.c, rcs.c, sanity.sh, subr.c, tag.c, and update.c
which I have only glanced at so far. However, I thought you
might be able to address the history.c questions.

Mark D. Baushke <mdb>
Group administrator
Wed 02 May 2007 02:51:05 PM UTC, original submission:  

sanity.sh tests run from valgrind (with default tool: memcheck) show memory leaks.

files provided include:

- patch: fix of leaks

- valgrind-cvs bash script to run instead of cvs (runs from src directory calls valgrind with cvs, redirects valgrind output to directory valgrind.log under src)

- patch: enable sanity.sh to work with valgrind-cvs


Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #12906:  valgrind.patch added by mdb (62KiB - text/x-patch - Fix memory leaks in FEATURE branch 1.12.13.1 and add --valgrind to sanity.sh)
file #12667:  cvs_fix_memory_leaks_1_12_13 added by None (15KiB - application/octet-stream)
file #12668:  valgrind-cvs added by None (306B - application/octet-stream)
file #12669:  cvs_sanity_valgrind_1_12_13.diff added by None (926B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by yyogev (Updated the item)
  • -email is unavailable- added by mdb (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-06-06 yyogev Attached File- Added patch_cvs_fix_memory_leaks_1_12_13_fix_hrecs_memset, #12966
    2007-06-04 yyogev Attached File- Added patch_cvs_fix_memory_leaks_1_12_13_b1.diff, #12944
    2007-05-30 mdb StatusNone Done
        Assigned toNone mdb
        Open/ClosedOpen Closed
        Fixed Feature ReleaseNone 1.12.14
    2007-05-30 mdb Attached File- Added valgrind.patch, #12906
    2007-05-02 None Attached File- Added cvs_fix_memory_leaks_1_12_13, #12667
        Attached File- Added valgrind-cvs, #12668
        Attached File- Added cvs_sanity_valgrind_1_12_13.diff, #12669

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code