[parted-devel] History patchset

[Jim Meyering]

Matt Davis <mattdavis9 at gmail.com> wrote:

> The following 3 patches implement history/undo/redo behavior for parted.
>
> This change captures potential disk modifications based on what commands a user
> issues.  The list of disk modifications can be displayed/queried, undone, or
> redone.  Eventually the changes can be commited to disk.  This prevents the user
> from performing unwanted modifications to disk before they are ready.  Also, the
> user can view the disk as if the modifications were taking place, even before
> the "save" their changes.
...
> diff --git a/libparted/history.c b/libparted/history.c
...
> +char PedHistStr[32];
...
> +PedHistManager PedHistMgr = {0};
...
> +void
> +ped_history_add (const char *name)
...
> +void
> +ped_history_clear (void)

Hi Matt,

I see at least two global variables there.
The existing ones cause enough trouble, (and this is library code),
so please don't add new global variables.

As I said before, you will want to adjust each of your history-manipulation
functions to take a pointer, probably *PedHistManager:

    void
    ped_history_add (PedHistManager *h, const char *name)

which brings up another point:
That function can fail (it calls malloc), hence it must not be void.

Please look carefully at any new function that is "void".
Each should probably should have a return type by which
to indicate success or failure.

Looking at ped_history_add, I see this:

    +    addme->name = ped_malloc (name_length);
    +    strncpy (addme->name, name, name_length);

if ped_malloc fails, it returns NULL.
If unchecked, strncpy will dereference that NULL,
with the likely result of a segfault.

Similarly for this:

    +    addme = ped_history_alloc_obj ();
    +    addme->id = PedHistMgr.id++;

Please audit your patch for any similar problems.