#146 formats.c - sox_append_comment - assert

Jim Harkins

In formats.c in the sox_append_comment function I see these lines:

*comments = lsx_realloc(*comments, (n + 2) * sizeof(**comments));
(*comments)[n++] = lsx_strdup(comment);

and it makes me think it should read:

*comments = lsx_realloc(*comments, (n + 2) * sizeof(**comments));
(*comments)[n++] = lsx_strdup(comment);

The existing version asserts that comments is not null but if it were null, the previous line would have already faulted. If the intent of the assert is to verify the allocation didn't fail, then my fix is correct.

I didn't create a patch because I'm not 100% positive what the intent was, and I'm not sure how to test this. My guess is the allocation never or almost never fails.



  • Doug Cook

    Doug Cook - 2009-09-26

    Good catch. I think the function should look something like this:

    size_t n = sox_num_comments(*comments);
    sox_comments_t new_comments = lsx_realloc(*comments, (n + 2) * sizeof(**comments));
    if (new_comments) {
    new_comments[n++] = lsx_strdup(comment);
    new_comments[n] = 0;
    *comments = new_comments;

  • robs

    robs - 2009-09-26

    In fact, the assert was put in to prevent the subsequent strdup from crashing from trying to copy a NULL pointer but is no longer needed since a) bugs have been fixed and b) lsx_strdup is now itself safe in this respect.

    The current alloc policy is that all allocs are done in a function called lsx_realloc; this will call exit if the alloc fails, so it is not necessary to check for alloc fails in application code.
    The idea was (IIRC) that a libSoX app could implement its own lsx_realloc if it wanted different behavior, but as Jim suggests, its probably a moot point.


  • Doug Cook

    Doug Cook - 2009-12-24
    • status: open --> closed-invalid
  • Doug Cook

    Doug Cook - 2009-12-24

    The assertion is on the "comment" parameter, not the "comments" variable. This assertion fails if the function is given a NULL parameter.


Log in to post a comment.