| Subject: | profile_flush_to_file() can corrupt shared tree state |
A profile data object can have two flags: DIRTY and SHARED. DIRTY means
there are unflushed changes in memory relative to the contents on disk, due to
a mutating API call like profile_update_relation(). SHARED means the data
object is part of g_shared_trees. These flags are exclusive; mutation
operations on a SHARED object will create a new data object with the DIRTY
flag.
The normal flush profile_flush() operation (which is also implicit in
profile_release()) checks the DIRTY flag and does nothing if it is not set.
There is no bug to report here.
profile_flush_to_file() writes the contents of the profile to a specified
file, which is presumably different from the one the profile was loaded from.
This calls profile_flush_file_data_to_file() which calls write_data_to_file().
The DIRTY flag is not checked, because there is no presumption that the target
file ever contained the contents of the data object.
At the end of write_data_to_file(), data->flags is set to 0. I assume the
intent is to clear the DIRTY flag for a profile_flush() call, but the flag
change is at the wrong layer. For profile_flush_to_file(), this is harmful in
two cases:
* For a DIRTY data object, the data object is set to non-DIRTY even though the
profile's backing file was probably not updated.
* For a SHARED data object, the data object remains part of g_shared_trees but
does not have the SHARED flag set. This becomes very bad later on. When the
data object is dereferenced and deleted, profile_free_file_data() will free
the data object but will not remove it from g_shared_trees, leaving a dangling
reference.
To reproduce the second problem:
1. Open a profile.
2. Without making any changes, call profile_flush_to_file().
3. Release the profile (leaves a dangling reference in g_shared_trees).
4. Open a profile (dereferences the dangling g_shared_trees reference and
fails an assertion).
Note that test1() in prof_test1 invokes profile_flush_to_file and then
profile_release. Fixing both sides bug will change the behavior of
profile_release (the updated relations will be written to test2.ini), so we
might need to change it to profile_abandon.
there are unflushed changes in memory relative to the contents on disk, due to
a mutating API call like profile_update_relation(). SHARED means the data
object is part of g_shared_trees. These flags are exclusive; mutation
operations on a SHARED object will create a new data object with the DIRTY
flag.
The normal flush profile_flush() operation (which is also implicit in
profile_release()) checks the DIRTY flag and does nothing if it is not set.
There is no bug to report here.
profile_flush_to_file() writes the contents of the profile to a specified
file, which is presumably different from the one the profile was loaded from.
This calls profile_flush_file_data_to_file() which calls write_data_to_file().
The DIRTY flag is not checked, because there is no presumption that the target
file ever contained the contents of the data object.
At the end of write_data_to_file(), data->flags is set to 0. I assume the
intent is to clear the DIRTY flag for a profile_flush() call, but the flag
change is at the wrong layer. For profile_flush_to_file(), this is harmful in
two cases:
* For a DIRTY data object, the data object is set to non-DIRTY even though the
profile's backing file was probably not updated.
* For a SHARED data object, the data object remains part of g_shared_trees but
does not have the SHARED flag set. This becomes very bad later on. When the
data object is dereferenced and deleted, profile_free_file_data() will free
the data object but will not remove it from g_shared_trees, leaving a dangling
reference.
To reproduce the second problem:
1. Open a profile.
2. Without making any changes, call profile_flush_to_file().
3. Release the profile (leaves a dangling reference in g_shared_trees).
4. Open a profile (dereferences the dangling g_shared_trees reference and
fails an assertion).
Note that test1() in prof_test1 invokes profile_flush_to_file and then
profile_release. Fixing both sides bug will change the behavior of
profile_release (the updated relations will be written to test2.ini), so we
might need to change it to profile_abandon.