Skip to content

Initialize pointers to NULL#651

Merged
cperciva merged 2 commits intomasterfrom
ptr-null
Mar 14, 2025
Merged

Initialize pointers to NULL#651
cperciva merged 2 commits intomasterfrom
ptr-null

Conversation

@gperciva
Copy link
Member

No description provided.

@gperciva
Copy link
Member Author

This is part 1 of 2: it resolves points which were not initialized to NULL as part of memset.*sizeof -- but not in libarchive/ code. Still working on that.

I'm a bit uncertain as to how much detail you want in these commits, and there's a whole "MAYBE" commit which adds an assertion to check a documented assumption about struct ccache_record. Might be overkill; happy to remove if it it's not necessary.

@gperciva gperciva changed the title Initialize pointers to NULL, plus related clean-up Initialize pointers to NULL Mar 5, 2025
@gperciva gperciva marked this pull request as ready for review March 5, 2025 21:18
@gperciva
Copy link
Member Author

gperciva commented Mar 5, 2025

This now includes initializing pointers in libarchive.

@gperciva gperciva marked this pull request as draft March 11, 2025 22:42
@gperciva gperciva marked this pull request as ready for review March 11, 2025 22:49
@cperciva
Copy link
Member

We should NULL pointers when we're relying on them being NULL. In some cases we were implicitly assuming that memset(0) set pointers to NULL, but we shouldn't make that assumption.

But there's no need to set all pointers to NULL unless there's some reason to think that it's going to matter (e.g. we're comparing against NULL or calling free on them later). And there's especially no point diverging from the upstream libarchive code to do that -- although if libarchive already has patches adding NULLification there's no harm in taking them.

@gperciva
Copy link
Member Author

We should NULL pointers when we're relying on them being NULL.

Right; those should be fixed.

Should I document each one in the commit message? (and obviously remove ones where I can't identify a specific error)

@cperciva
Copy link
Member

We should NULL pointers when we're relying on them being NULL.

Right; those should be fixed.

Should I document each one in the commit message? (and obviously remove ones where I can't identify a specific error)

Yes please.

archive_entry_link_resolver.c: we need res->spare to be NULL before we
    call find_entry() or next_entry().

tree.c: if we call tree_open() and then tree_close(), we need t->buff to
    be NULL.

Reported by:	Ted Unangst
Bug bounty:	$2
bsdtar.c:
  - clarification: sort initializations in the same order as their
    declaration in bsdtar.h.

matching.c:
  - suppose we call inclusions() and then cleanup_exclusions().
    inclusions() then calls initialize_matching() and then add_pattern()
    which initializes matching->inclusions.
    However, matching->exclusions was only initialized with a memset(),
    so it might not be NULL when cleanup_exclusions() does
    `p = bsdtar->matching->exclusions; while (p != NULL)`.
    The same argument applies if we call exclusions() instead of
    inclusions() at the beginning.

tree.c:
  - t->stack: if we call tree_open() and then tree_close(), we need it
    to be NULL for the `while (t->stack != NULL)`.
  - t->d: if we call tree_open() and then tree_next(), we need it to be
    NULL for the `while (t->d != NULL)`.
@gperciva
Copy link
Member Author

Revised with justifications in commit messages, and omitting the non-bounty libarchive/ changes. A subset of this might come in a later PR.

@cperciva cperciva merged commit 9ddeafd into master Mar 14, 2025
2 checks passed
@gperciva gperciva deleted the ptr-null branch March 14, 2025 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants