#42 Few cosmetic cleanups and memory allocation/freeing fixes

open
None
5
2007-06-02
2006-10-10
No

Hello,

In addition to a few cosmetic fixes in configure.in,
asm.c and entry.c, this
patch fixes a few 'allocated-but-not-freed' issues I've
found in ctags 5.6.
Although ctags is not a server which may be running in
a weeks and thrashing
all system memory, being an absolutely malloc()/free()
correct is a good
idea IMHO. Some non-freed pointers were detected by
Valgrind, and others
were hunted with the help of a toy vString leak error
detector which is
implemented as a part of vString code.

I've also checked that this patch may be applied (with
1 hunk in c.c)
to the latest SVN trunk (revision 469).

Dmitry

Discussion

  • Dmitry Antipov

    Dmitry Antipov - 2006-10-10

    patch

     
  • Elliott Hughes

    Elliott Hughes - 2007-06-02
    • assigned_to: nobody --> elliotth
     
  • Elliott Hughes

    Elliott Hughes - 2007-06-02

    Logged In: YES
    user_id=1127237
    Originator: NO

    thanks for that! i've committed your leak fixes, plus others you either missed or which weren't present when you took a look. a few remain (in "fortran.c" and "sql.c", plus "parseLongOption") because i don't have nice fixes for them.

    could you explain the configure-script changes? what do they improve?

     
  • Elliott Hughes

    Elliott Hughes - 2007-06-02

    Logged In: YES
    user_id=1127237
    Originator: NO

    here, for reference, was my check-in comment:

    2007-06-01 23:09:00 -0700 elliotth (536)

    trunk/args.c:
    trunk/asm.c:
    trunk/beta.c:
    trunk/eiffel.c:
    trunk/fortran.c:
    trunk/jscript.c:
    trunk/lregex.c:
    trunk/main.c:
    trunk/options.c:
    trunk/pascal.c:
    trunk/read.c:
    trunk/routines.c:
    trunk/routines.h:
    trunk/sml.c:
    trunk/sql.c: fix almost all our current memory leaks. Based on a patch from Dmitry Antipov, but also using his vString leak detector and valgrind(1) to find new ones and ones he missed. Three known leaks remain. The first is in parseLongOption. There's also one in "fortran.c" and another in "sql.c":

    helium:~/Projects/ctags/trunk$ valgrind --leak-check=full --show-reachable=yes ./dctags -f - Test/*
    ==3056== Memcheck, a memory error detector.
    ==3056== Copyright (C) 2002-2006, and GNU GPL'd, by Julian Seward et al.
    ==3056== Using LibVEX rev 1658, a library for dynamic binary translation.
    ==3056== Copyright (C) 2004-2006, and GNU GPL'd, by OpenWorks LLP.
    ==3056== Using valgrind-3.2.1-Debian, a dynamic binary instrumentation framework.
    ==3056== Copyright (C) 2000-2006, and GNU GPL'd, by Julian Seward et al.
    ==3056== For more details, rerun with: -v
    ==3056==
    .
    .
    .
    ==3056==
    ==3056== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 13 from 1)
    ==3056== malloc/free: in use at exit: 708 bytes in 22 blocks.
    ==3056== malloc/free: 36,126 allocs, 36,104 frees, 1,584,216 bytes allocated.
    ==3056== For counts of detected errors, rerun with: -v
    ==3056== searching for pointers to 22 not-freed blocks.
    ==3056== checked 68,184 bytes.
    ==3056==
    ==3056== 68 bytes in 2 blocks are definitely lost in loss record 1 of 2
    ==3056== at 0x4021620: malloc (vg_replace_malloc.c:149)
    ==3056== by 0x806347E: eMalloc (routines.c:238)
    ==3056== by 0x8065B68: newToken (sql.c:347)
    ==3056== by 0x80662BB: parseSubProgram (sql.c:688)
    ==3056== by 0x8067867: parseSqlFile (sql.c:1782)
    ==3056== by 0x8067934: findSqlTags (sql.c:1810)
    ==3056== by 0x8060760: createTagsForFile (parse.c:618)
    ==3056== by 0x8060810: createTagsWithFallback (parse.c:640)
    ==3056== by 0x80608DC: parseFile (parse.c:667)
    ==3056== by 0x805B7D6: createTagsForEntry (main.c:303)
    ==3056== by 0x805B811: createTagsForArgs (main.c:348)
    ==3056== by 0x805BD6F: makeTags (main.c:494)
    ==3056==
    ==3056==
    ==3056== 640 bytes in 20 blocks are still reachable in loss record 2 of 2
    ==3056== at 0x4021620: malloc (vg_replace_malloc.c:149)
    ==3056== by 0x806347E: eMalloc (routines.c:238)
    ==3056== by 0x806AEE2: vStringNew (vstring.c:116)
    ==3056== by 0x80542C0: newToken (fortran.c:419)
    ==3056== by 0x8054309: newTokenFrom (fortran.c:429)
    ==3056== by 0x80562E5: parseInterfaceBlock (fortran.c:1709)
    ==3056== by 0x805661D: parseDeclarationConstruct (fortran.c:1834)
    ==3056== by 0x805679F: parseSpecificationPart (fortran.c:1901)
    ==3056== by 0x80569F5: parseModule (fortran.c:1990)
    ==3056== by 0x8056E05: parseProgramUnit (fortran.c:2142)
    ==3056== by 0x8056F37: findFortranTags (fortran.c:2183)
    ==3056== by 0x806077A: createTagsForFile (parse.c:620)
    ==3056==
    ==3056== LEAK SUMMARY:
    ==3056== definitely lost: 68 bytes in 2 blocks.
    ==3056== possibly lost: 0 bytes in 0 blocks.
    ==3056== still reachable: 640 bytes in 20 blocks.
    ==3056== suppressed: 0 bytes in 0 blocks.

    I think they're both awkward longjmp(3)/setjmp(3)-related leaks, and I don't currently have a good solution. ("eiffel.c" cunningly only calls newToken once, before calling setjmp(3).)

     
  • Elliott Hughes

    Elliott Hughes - 2007-06-02

    Logged In: YES
    user_id=1127237
    Originator: NO

    here's what i haven't done anything with yet:

    1. can you supply test cases that show these leaks? i've run ctags on our test cases plus the 2.6.17 Linux kernel source, and seen no "c.c" leaks.

    diff -ur ctags-5.6/c.c ctags-miscfix-5.6/c.c
    --- ctags-5.6/c.c 2006-05-30 08:37:11.000000000 +0400
    +++ ctags-miscfix-5.6/c.c 2006-10-10 16:50:34.000000000 +0400
    @@ -460,6 +460,9 @@
    { "while", KEYWORD_WHILE, { 1, 1, 1, 1, 0 } }
    };

    +/* Tokens which may be clobbered by longjmp() and not freed. */
    +static tokenInfo *jumpClobbered[2];
    +
    /*
    * FUNCTION PROTOTYPES
    */
    @@ -1647,6 +1650,8 @@
    tokenInfo *const parent = newToken ();
    int c;

    + jumpClobbered[0] = token;
    + jumpClobbered[1] = parent;
    do
    {
    c = skipToNonWhite ();
    @@ -1674,6 +1679,7 @@
    cppUngetc (c);
    deleteToken (parent);
    deleteToken (token);
    + jumpClobbered[0] = jumpClobbered[1] = NULL;
    }

    static void skipStatement (statementInfo *const st)
    @@ -2510,6 +2516,14 @@
    {
    while (CurrentStatement != NULL)
    deleteStatement ();
    + if (jumpClobbered[0] != NULL) {
    + deleteToken (jumpClobbered[0]);
    + jumpClobbered[0] = NULL;
    + }
    + if (jumpClobbered[1] != NULL) {
    + deleteToken (jumpClobbered[1]);
    + jumpClobbered[1] = NULL;
    + }
    }

    static boolean isStatementEnd (const statementInfo *const st)

    2. i don't understand enough about autoconf to know why we'd want to make these changes:

    diff -ur ctags-5.6/configure.in ctags-miscfix-5.6/configure.in
    --- ctags-5.6/configure.in 2006-05-30 08:52:39.000000000 +0400
    +++ ctags-miscfix-5.6/configure.in 2006-10-10 09:09:34.000000000 +0400
    @@ -21,7 +21,7 @@
    dnl CHECK_HEADER_DEFINE(LABEL, HEADER [,ACTION-IF-FOUND [, ACTION-IF-NOT-FOUND ] ])
    AC_DEFUN(CHECK_HEADER_DEFINE,
    [
    - AC_MSG_CHECKING("if $1 is defined in $2")
    + AC_MSG_CHECKING(if $1 is defined in $2)
    AC_EGREP_CPP(yes,
    [#include <$2>
    #ifdef $1
    @@ -123,7 +123,7 @@

    case `uname` in
    HP-UX)
    - AC_MSG_CHECKING("HP-UX native compiler")
    + AC_MSG_CHECKING(HP-UX native compiler)
    if test "$CC" = "cc"; then
    AC_MSG_RESULT(yes; adding compiler options for ANSI support)
    CFLAGS="$CFLAGS -Aa -D_HPUX_SOURCE"
    @@ -133,7 +133,7 @@
    ;;
    SunOS)
    if uname -r | grep '5\.' >/dev/null 2>&1; then
    - AC_MSG_CHECKING("Solaris native compiler")
    + AC_MSG_CHECKING(Solaris native compiler)
    if test "$CC" = "cc" -a "`which cc`" = "/usr/ucb/cc"; then
    AC_MSG_RESULT(yes; adding compiler option for ANSI support)
    CC="$CC -Xa"
    @@ -182,7 +182,7 @@
    fi

    dnl Test for case-insensitive filenames
    -AC_MSG_CHECKING("for case-insensitive filenames")
    +AC_MSG_CHECKING(for case-insensitive filenames)
    touch conftest.cif
    if test -f CONFTEST.CIF; then
    AC_MSG_RESULT(yes)
    @@ -242,14 +242,14 @@
    AC_TYPE_SIZE_T
    AC_TYPE_OFF_T

    -AC_MSG_CHECKING("for fpos_t")
    +AC_MSG_CHECKING(for fpos_t)
    AC_EGREP_HEADER(fpos_t, stdio.h, AC_MSG_RESULT(yes),
    [
    AC_MSG_RESULT(no)
    AC_DEFINE(fpos_t, long)
    ])

    -AC_MSG_CHECKING("for clock_t")
    +AC_MSG_CHECKING(for clock_t)
    AC_EGREP_HEADER(clock_t, time.h, AC_MSG_RESULT(yes),
    [
    AC_MSG_RESULT(no)
    @@ -328,7 +328,7 @@
    if test "$have_putenv" = yes ; then
    AC_EGREP_HEADER(putenv, stdlib.h, have_putenv_prototype=yes)
    if test "$have_putenv_prototype" = yes ; then
    - AC_MSG_CHECKING("putenv prototype")
    + AC_MSG_CHECKING(putenv prototype)
    AC_EGREP_HEADER([[^A-Za-zo-9_]putenv[ ]*\(.*const.*\)[ ]*;],
    stdlib.h, AC_MSG_RESULT(correct),
    [
    @@ -382,7 +382,7 @@

    dnl Checks for missing prototypes
    dnl -----------------------------
    -AC_CHECKING("for missing prototypes")
    +AC_CHECKING(for missing prototypes)

    AC_DEFUN(CHECK_PROTO, [AC_EGREP_HEADER([[^A-Za-z0-9_]$1([ ]+[A-Za-z0-9_]*)?\(], $2,, AC_DEFINE(patsubst([NEED_PROTO_NAME], [NAME], translit($1, [a-z], [A-Z]))) AC_MSG_RESULT(adding prototype for $1))])

    3. i don't think the benefit of saving a couple of calls to strlen(3) is worth the extra cruft here, even though TagFile.max.tag and TagFile.max.line do seem to be wholly unused:

    diff -ur ctags-5.6/entry.c ctags-miscfix-5.6/entry.c
    --- ctags-5.6/entry.c 2006-05-30 08:37:12.000000000 +0400
    +++ ctags-miscfix-5.6/entry.c 2006-10-10 09:09:34.000000000 +0400
    @@ -117,7 +117,7 @@
    /*
    * Pseudo tag support
    */
    -
    +#ifdef DEBUG
    static void rememberMaxLengths (const size_t nameLength, const size_t lineLength)
    {
    if (nameLength > TagFile.max.tag)
    @@ -126,17 +126,20 @@
    if (lineLength > TagFile.max.line)
    TagFile.max.line = lineLength;
    }
    +#endif

    static void writePseudoTag (
    const char *const tagName,
    const char *const fileName,
    const char *const pattern)
    {
    - const int length = fprintf (
    - TagFile.fp, "%s%s\t%s\t/%s/\n",
    - PSEUDO_TAG_PREFIX, tagName, fileName, pattern);
    +#ifdef DEBUG
    + const int length =
    +#endif
    + fprintf (TagFile.fp, "%s%s\t%s\t/%s/\n",
    + PSEUDO_TAG_PREFIX, tagName, fileName, pattern);
    ++TagFile.numTags.added;
    - rememberMaxLengths (strlen (tagName), (size_t) length);
    + DebugStatement ( rememberMaxLengths (strlen (tagName), (size_t) length); )
    }

    static void addPseudoTags (void)
    @@ -390,7 +393,6 @@
    {
    boolean fileExists;

    - setDefaultTagFileName ();
    TagFile.name = eStrdup (Option.tagFileName);
    fileExists = doesFileExist (TagFile.name);
    if (fileExists && ! isTagFile (TagFile.name))
    @@ -827,7 +829,7 @@
    length = writeCtagsEntry (tag);

    ++TagFile.numTags.added;
    - rememberMaxLengths (strlen (tag->name), (size_t) length);
    + DebugStatement ( rememberMaxLengths (strlen (tag->name), (size_t) length); )
    DebugStatement ( fflush (TagFile.fp); )
    }
    }

    4. i haven't committed your vString leak detector (though i'm still running with a variant of it) because valgrind(1) seems like a better solution. non-Linux users might disagree. probably worth discussing on the list.

     
  • Elliott Hughes

    Elliott Hughes - 2007-06-24

    Logged In: YES
    user_id=1127237
    Originator: NO

    i've committed the "configure.in" changes. the difference is between:

    checking "for case-insensitive filenames"... no

    and:

    checking for case-insensitive filenames... no

     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks