Notice: We have migrated to GitLab launching 2024-05-01 see here: https://gitlab.rtems.org/

#4212 closed defect (invalid)

libio leaks location clones

Reported by: Chris Johns Owned by: Chris Johns
Priority: normal Milestone: 6.1
Component: fs Version: 6
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

rtems_filesystem_location_copy overwrites a location that may have been cloned via the ops handler leaking the destination node. If a file system has allocated resources in a location node when cloned it will leak. I assume there is a free for every clone?

An example of a leak is to mount of file system under IMFS and then open a file under the mounted file system. The path eval restart in the IMFS will clone a node and then open will:

  rtems_filesystem_eval_path_extract_currentloc( &ctx, &iop->pathinfo );
  rtems_filesystem_eval_path_cleanup( &ctx );

The ctx contains a cloned location.

Allocating resources to a location may be required because of the limited available file system fields and the opaque nature of the fields or a file system may have data it locks or reference counts and that needs to be managed.

I do not fully understand the semantics around the location object and while I could guess at a fix there are a number functions related to locations and a large number of use cases that I would not know if my fix is OK. For example should the copy function free the destination location and then clone the source location or should it just free the destination location or should all the places a copy is done be updated to manage the free and cloning based on the context? Or is the issue in the open call and the lines pasted here be swapped?

Change History (9)

comment:1 Changed on 01/07/21 at 08:16:42 by Sebastian Huber

Do we have this resource leak with one of the current file systems? If yes, could you please add test case for this resource leak.

comment:2 in reply to:  1 Changed on 01/07/21 at 22:20:27 by Chris Johns

Replying to Sebastian Huber:

Do we have this resource leak with one of the current file systems?

Yes I believe there is a leak with open as stated above.

If yes, could you please add test case for this resource leak.

A test is a good idea however as stated I do not understand the locations struct and the eval code in libio. I could create a test and provide a fix and break an existing file system.

Maybe the following will help explain why I am confused:

  1. Is this simply a bug in open? Does ...
      rtems_filesystem_eval_path_cleanup( &ctx );
      rtems_filesystem_eval_path_extract_currentloc( &ctx, &iop->pathinfo );
    
    ... resolve the leak? Does that create another leak and so should the code be ...
      rtems_filesystem_eval_path_cleanup( &ctx );
      rtems_filesystem_eval_path_extract_currentloc( &ctx, &iop->pathinfo );
      rtems_filesystem_eval_path_cleanup( &ctx );
    
    ... ? Would fixing the copy also resolve this leak? I have no idea which of these is right solution because I do not understand how this is suppose to work.
  1. The call rtems_filesystem_location_copy() does not free the resource. The only doco with this function says it is a bitwise copy yet the resource will be added to the corresponding mount. This is confusing because the resource is accounted for yet it does not perform a deep copy of the contents when the file system is required to provide an interface to do this.
Last edited on 01/07/21 at 23:54:46 by Chris Johns (previous) (diff)

comment:3 Changed on 01/08/21 at 05:28:40 by Sebastian Huber

I am not sure if we have an unintentional resource leak in open() with the current file systems. I would be good to have a test case. The purpose of open() is to create a location for a file which is released to the system through a call to close(). If errors happen during the path evaluation, the rtems_filesystem_eval_path_error() should be called which uses rtems_filesystem_location_detach() if necessary.

comment:4 Changed on 01/09/21 at 01:24:43 by Chris Johns

I agree we need a test, there is no objection here. I will look at this when I have time.

The bug is in the eval restart call in the IMFS. When a mount point is found the eval restart call clones the root loc. The open code ends up calling copy which wipes the clone out. A test is needed to expose the leak.

Putting the test case side, I would like to know if the copy should be deep? The test could then check for it being shallow or deep.

comment:5 Changed on 01/11/21 at 05:25:37 by Sebastian Huber

From looking at the code, I don't see an issue in the restart code:

void rtems_filesystem_eval_path_restart(
  rtems_filesystem_eval_path_context_t *ctx,
  rtems_filesystem_global_location_t **newstartloc_ptr
)
{
  free_location(&ctx->currentloc);
  rtems_filesystem_instance_unlock(&ctx->startloc->location);
  rtems_filesystem_global_location_assign(
    &ctx->startloc,
    rtems_filesystem_global_location_obtain(newstartloc_ptr)
  );
  rtems_filesystem_instance_lock(&ctx->startloc->location);
  rtems_filesystem_location_clone(&ctx->currentloc, &ctx->startloc->location);
}

It first frees the current location and then continues with a clone of the new start location. A test case would definitely help.

The locations are just pointers which are registered in the system. I think the documentation of rtems_filesystem_location_copy() is in line with its implementation.

Last edited on 01/12/21 at 01:25:10 by Chris Johns (previous) (diff)

comment:6 Changed on 01/12/21 at 01:36:02 by Chris Johns

It might have been the optimiser had not shown the path in the debugger and I was seeing something else. A review of this and then the path clean up in open does seem OK. The struct vnode counts will highlight an issue once I have that integrated. When I saw the problem and noted it I did not see the free but I saw the copy.

comment:7 Changed on 12/17/21 at 17:05:49 by Joel Sherrill

Is this still an issue?

comment:8 Changed on 01/26/22 at 13:54:33 by Sebastian Huber

Owner: set to Chris Johns
Status: newassigned

Please close if possible.

comment:9 Changed on 11/29/22 at 22:27:24 by Chris Johns

Resolution: invalid
Status: assignedclosed

Please reopen if this is still a valid issue.

Note: See TracTickets for help on using tickets.