Incorrect C example

[mitjap]

The following example copies detection data in a pointer type (apriltag_detection_t *) instead to value type (apriltag_detection_t). This causes the zarray_get to write to memory on stack outside its bounds and can cause stack corruption. I think this is incorrect as zarray_get expects a pointer to data structure not a pointer to a pointer. This code has undefined behavior. Also cleanup function (apriltag_detections_destroy) is missing at the end as specified in documentation.

for (int i = 0; i < zarray_size(detections); i++) {
    apriltag_detection_t *det;
    zarray_get(detections, i, &det);

    // Do stuff with detections here.
}
// Cleanup.
tagStandard41h12_destroy(tf);
apriltag_detector_destroy(td);

I think this would be correct example:

for (int i = 0; i < zarray_size(detections); i++) {
    apriltag_detection_t det;
    zarray_get(detections, i, &det);

    // Do stuff with detections here.
}
// Cleanup.
apriltag_detections_destroy(detections);
tagStandard41h12_destroy(tf);
apriltag_detector_destroy(td);

[christian-rauch]

I am missing some context. Is this code from a file in the repo?

If so, is this pointer issue reported by ASAN?

[mitjap]

This example is from https://github.com/AprilRobotics/apriltag/wiki/AprilTag-User-Guide#c.

I did not check with address sanitiser.

[christian-rauch]

I didn't know about that wiki. It seems to be outdated. The python binding the wiki refers to (https://github.com/duckietown/apriltags3-py) are not maintained any more.

You are right about the pointer access. I am not sure how to send PRs to the wiki. Can you try to edit the wiki?

[mitjap]

Sorry I also can not edit the Wiki.

@mkrogius can you help with this?

[mitjap]

My first claim that this is undefined behaviour is incorrect. detections is array of pointers so this part of example is correct.

Only issue with example is memory leak due to missing apriltag_detections_destroy call.

[dougbalish1]

@mitjap @christian-rauch Not sure if this is worth reporting with the state of things but figured this was the most relevant issue open.

I was having a pretty annoying issue using the order of things from the wiki C example, required valgrind to chase it down because it doesn't seem to show up unless you allocate more memory afterwards;

int main() {
  // Set up apriltag detectors 
  apriltag_detector_t *td = apriltag_detector_create();
  apriltag_family_t *tf = tag36h11_create();
  apriltag_detector_add_family(td, tf);

  std::cout << "Destroying tag detectors\n";
  tag36h11_destroy(tf);
  apriltag_detector_destroy(td);
  std::cout << "Returning from test\n";

  return 0;
}

Causes valgrind to find the following invalid operations:

Destroying tag detectors
==58584== Invalid read of size 8
==58584==    at 0x4879988: apriltag_detector_clear_families (in /usr/local/lib/libapriltag.so.3.3.0)
==58584==    by 0x4879B28: apriltag_detector_destroy (in /usr/local/lib/libapriltag.so.3.3.0)
==58584==    by 0x1092F3: main (main.cc:17)
==58584==  Address 0x4f5aea0 is 64 bytes inside a block of size 72 free'd
==58584==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==58584==    by 0x1092E7: main (main.cc:16)
==58584==  Block was alloc'd at
==58584==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==58584==    by 0x489D529: tag36h11_create (in /usr/local/lib/libapriltag.so.3.3.0)
==58584==    by 0x1092B1: main (main.cc:12)
==58584== 
==58584== Invalid write of size 8
==58584==    at 0x48799A6: apriltag_detector_clear_families (in /usr/local/lib/libapriltag.so.3.3.0)
==58584==    by 0x4879B28: apriltag_detector_destroy (in /usr/local/lib/libapriltag.so.3.3.0)
==58584==    by 0x1092F3: main (main.cc:17)
==58584==  Address 0x4f5aea0 is 64 bytes inside a block of size 72 free'd
==58584==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==58584==    by 0x1092E7: main (main.cc:16)
==58584==  Block was alloc'd at
==58584==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==58584==    by 0x489D529: tag36h11_create (in /usr/local/lib/libapriltag.so.3.3.0)
==58584==    by 0x1092B1: main (main.cc:12)
==58584== 
Returning from test

This doesn't cause immediate issues, but down the line it can cause a crash as alloc()/free() detects corruption (in my case at least).

The fix would be very simple, the order of family and detector destruction just need to be swapped in the example

[christian-rauch]

The wiki is now part of the repo. You can send a PR to update the C example.

[Tnze]

There is a memory leak in the C example too. The detections is not destroyed.

-----Edit----- Oh, saw this is mentioned on this issue's owner.

[christian-rauch]

There is a memory leak in the C example too. The detections is not destroyed.

-----Edit----- Oh, saw this is mentioned on this issue's owner.

Can you send a PR with the fix?

[Tnze]

There is a memory leak in the C example too. The detections is not destroyed. -----Edit----- Oh, saw this is mentioned on this issue's owner.

Can you send a PR with the fix?

Of course

[Tnze]

My first claim that this is undefined behaviour is incorrect. detections is array of pointers so this part of example is correct.

You said it is correct.

[mitjap]

You are correct. I deleted my comment just after posting.