Skip to content

Commit

Permalink
Replace magic number with constant DEVLIST_ALLOC_INITIAL_LEN & fix
Browse files Browse the repository at this point in the history
memory leaks in calling `fido_dev_info_free`

* Add preprocessor constant `DEVLIST_ALLOC_INITIAL_LEN` to replace magic
number 64 in allocations of device info list. The named constant
expresses meaning and makes bugs & memory-leaks due to missed code
changes less likely, should one decide to change the number of slots to
allocate in the device info list.

* Fix the same errorneous code in freeing the device info list at three
points in the code:
Calling `fido_dev_info_free` with length argument `ndevs` deallocates
only the filled entries of the device info list, while leaving the empty
entries dangling. This does not seem to leak any data, though.
The commit fixes the code to actually deallocate the whole array.
  • Loading branch information
eumpf0 committed Jan 4, 2024
1 parent 1274d94 commit c738182
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 11 deletions.
8 changes: 4 additions & 4 deletions pamu2fcfg/pamu2fcfg.c
Original file line number Diff line number Diff line change
Expand Up @@ -467,13 +467,13 @@ int main(int argc, char *argv[]) {
parse_args(argc, argv, &args);
fido_init(args.debug ? FIDO_DEBUG : 0);

devlist = fido_dev_info_new(64);
devlist = fido_dev_info_new(DEVLIST_ALLOC_INITIAL_LEN);
if (!devlist) {
fprintf(stderr, "error: fido_dev_info_new failed\n");
goto err;
}

r = fido_dev_info_manifest(devlist, 64, &ndevs);
r = fido_dev_info_manifest(devlist, DEVLIST_ALLOC_INITIAL_LEN, &ndevs);
if (r != FIDO_OK) {
fprintf(stderr, "Unable to discover device(s), %s (%d)\n", fido_strerr(r),
r);
Expand All @@ -489,7 +489,7 @@ int main(int argc, char *argv[]) {
fflush(stderr);
sleep(FREQUENCY);

r = fido_dev_info_manifest(devlist, 64, &ndevs);
r = fido_dev_info_manifest(devlist, DEVLIST_ALLOC_INITIAL_LEN, &ndevs);
if (r != FIDO_OK) {
fprintf(stderr, "\nUnable to discover device(s), %s (%d)",
fido_strerr(r), r);
Expand Down Expand Up @@ -564,7 +564,7 @@ int main(int argc, char *argv[]) {
err:
if (dev != NULL)
fido_dev_close(dev);
fido_dev_info_free(&devlist, ndevs);
fido_dev_info_free(&devlist, DEVLIST_ALLOC_INITIAL_LEN);
fido_cred_free(&cred);
fido_dev_free(&dev);

Expand Down
14 changes: 7 additions & 7 deletions util.c
Original file line number Diff line number Diff line change
Expand Up @@ -1155,13 +1155,13 @@ int do_authentication(const cfg_t *cfg, const device_t *devices,
#endif
memset(&pk, 0, sizeof(pk));

devlist = fido_dev_info_new(64);
devlist = fido_dev_info_new(DEVLIST_ALLOC_INITIAL_LEN);
if (!devlist) {
debug_dbg(cfg, "Unable to allocate devlist");
goto out;
}

r = fido_dev_info_manifest(devlist, 64, &ndevs);
r = fido_dev_info_manifest(devlist, DEVLIST_ALLOC_INITIAL_LEN, &ndevs);
if (r != FIDO_OK) {
debug_dbg(cfg, "Unable to discover device(s), %s (%d)", fido_strerr(r), r);
goto out;
Expand All @@ -1171,7 +1171,7 @@ int do_authentication(const cfg_t *cfg, const device_t *devices,

debug_dbg(cfg, "Device max index is %zu", ndevs);

authlist = calloc(64 + 1, sizeof(fido_dev_t *));
authlist = calloc(DEVLIST_ALLOC_INITIAL_LEN + 1, sizeof(fido_dev_t *));
if (!authlist) {
debug_dbg(cfg, "Unable to allocate authenticator list");
goto out;
Expand Down Expand Up @@ -1265,15 +1265,15 @@ int do_authentication(const cfg_t *cfg, const device_t *devices,

i++;

fido_dev_info_free(&devlist, ndevs);
fido_dev_info_free(&devlist, DEVLIST_ALLOC_INITIAL_LEN);

devlist = fido_dev_info_new(64);
devlist = fido_dev_info_new(DEVLIST_ALLOC_INITIAL_LEN);
if (!devlist) {
debug_dbg(cfg, "Unable to allocate devlist");
goto out;
}

r = fido_dev_info_manifest(devlist, 64, &ndevs);
r = fido_dev_info_manifest(devlist, DEVLIST_ALLOC_INITIAL_LEN, &ndevs);
if (r != FIDO_OK) {
debug_dbg(cfg, "Unable to discover device(s), %s (%d)", fido_strerr(r),
r);
Expand All @@ -1299,7 +1299,7 @@ int do_authentication(const cfg_t *cfg, const device_t *devices,
out:
reset_pk(&pk);
fido_assert_free(&assert);
fido_dev_info_free(&devlist, ndevs);
fido_dev_info_free(&devlist, DEVLIST_ALLOC_INITIAL_LEN);

if (authlist) {
for (size_t j = 0; authlist[j] != NULL; j++) {
Expand Down
2 changes: 2 additions & 0 deletions util.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#define DEFAULT_ORIGIN_PREFIX "pam://"
#define SSH_ORIGIN "ssh:"

#define DEVLIST_ALLOC_INITIAL_LEN 64

typedef struct {
unsigned max_devs;
int manual;
Expand Down

0 comments on commit c738182

Please sign in to comment.