bugConcurrent Versions System - Bugs: bug #36276, Potential problem in...

 
 

bug #36276: Potential problem in parse_config() which may leak file descriptor

Submitter:  None
Submitted:  Mon 23 Apr 2012 08:26:36 PM UTC
   
 
Category:  Bug Report Severity:  3 - Normal
Item Group:  None Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Release: 
Fixed Release:  None Fixed Feature Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Wed 25 Apr 2012 06:26:46 AM UTC, comment #1: 

All parse_config() calls do not check return value, run_exec() does not close
unneeded descriptors and CVS_FOPEN does not set O_CLOEXEC, so there is
possibility external command gets access to CVS configuration file.

I think copying final fclose() after set_defaults_and_return label is the best
solution. Move is not enough because the non-error path would return without
closing the file.

Petr Pisar <petrp>
Mon 23 Apr 2012 08:26:36 PM UTC, original submission:  

We are developing a tool to find bugs, and it flagged a potential error in CVS.
We are checking CVS version 1.11.23.
In parse_config() in src/parseinfo.c, the config file is opened at line 274:

274:    fp_info = CVS_FOPEN (infopath, "r");

The file is closed if everything goes well:

461:    if (fclose (fp_info) < 0)

But if there is a syntax error, the file is not closed:

321:    p = strchr (line, '=');
322:    if (p == NULL)
323:    {
323:        /* Probably should be printing line number.  */
324:        error (0, 0, "syntax error in %s: line '%s' is missing '='",
325:               infopath, line);
326:        goto error_return;
327:    }
...
474: error_return:
475:    if (!logHistory)
476:    logHistory = xstrdup (ALL_HISTORY_REC_TYPES);
477:    if (infopath != NULL)
478:    free (infopath);
479:    if (line != NULL)
480:    free (line);
          // Shall we have something like
          // if (fp_info) fclose(fp_info);
          // here?
481:    return -1;
482:}

Since the problem is due to a syntax error, shall we still close the file?
We may just move the call to fclose() into the error_return block.
Else the file would be left opened after the function returns.

Is this a real problem? Any confirmation or clarification is
appreciated. Thanks.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #44074:  cvs-1.11.23-Close-a-configuration-file-on-a-syntax-error.patch added by petrp (977B - application/octet-stream - Proposed fix)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by petrp (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.

    Only logged-in users can vote.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-05-03 petrp Attached File- Added cvs-1.11.23-Close-a-configuration-file-on-a-syntax-error.patch, #44074

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code