Created attachment 357436 [details] [review]
tests: Add more assertions in the tests

Add assertions to various syscalls which could fail (and would mess up
the test state if they did), as spotted by Coverity.

Fixes CIDs: 162185, 162186, 162188–162191.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Review of attachment 357436 [details] [review]:

Thanks for doing this :). Looks good to me.

::: tests/libtracker-common/tracker-file-utils-test.c
@@ +39,3 @@
 ensure_file_exists (const gchar *filename)
 {
+        g_autoptr(GError) error = NULL;

Just want to point out that it's a first in Tracker :). We already depend on glib 2.44, so I've got nothing against it.

(In reply to Carlos Garnacho from comment #2)
> Review of attachment 357436 [details] [review] [review]:
> 
> Thanks for doing this :). Looks good to me.

:-)

Note there’s still one issue left in Coverity for tracker-miners, which needs more detailed attention. I’m not sure when I’ll get time to take a look.

> ::: tests/libtracker-common/tracker-file-utils-test.c
> @@ +39,3 @@
>  ensure_file_exists (const gchar *filename)
>  {
> +        g_autoptr(GError) error = NULL;
> 
> Just want to point out that it's a first in Tracker :). We already depend on
> glib 2.44, so I've got nothing against it.

Wheeee! I hope it’s the first of many. I’ve almost forgotten how to code without g_autoptr(). The future’s almost bearable!

Attachment 357436 [details] pushed as ef8da6f - tests: Add more assertions in the tests

(In reply to Philip Withnall from comment #3)
> (In reply to Carlos Garnacho from comment #2)
> > Review of attachment 357436 [details] [review] [review] [review]:
> > 
> > Thanks for doing this :). Looks good to me.
> 
> :-)
> 
> Note there’s still one issue left in Coverity for tracker-miners, which
> needs more detailed attention. I’m not sure when I’ll get time to take a
> look.

Yeah... I did try to address this on commits ccf5b4b5 and 04c0484c5 to no avail. I think it'll just taint any byte swapping regardless on the checks done afterwards. But that's just how ID3v2 works, stuff is stored with most significant byte first...

(In reply to Carlos Garnacho from comment #5)
> (In reply to Philip Withnall from comment #3)
> > (In reply to Carlos Garnacho from comment #2)
> > > Review of attachment 357436 [details] [review] [review] [review] [review]:
> > > 
> > > Thanks for doing this :). Looks good to me.
> > 
> > :-)
> > 
> > Note there’s still one issue left in Coverity for tracker-miners, which
> > needs more detailed attention. I’m not sure when I’ll get time to take a
> > look.
> 
> Yeah... I did try to address this on commits ccf5b4b5 and 04c0484c5 to no
> avail. I think it'll just taint any byte swapping regardless on the checks
> done afterwards. But that's just how ID3v2 works, stuff is stored with most
> significant byte first...

The problem is that all of those calculations might overflow, I think.

(pos + frame_size + csize) might overflow
(pos + csize) might overflow
(tsize + header_size) might overflow

GLib provides g_size_checked_add() for this, but it’s a bit of a pain to use. The approach I normally take is to rearrange the arithmetic. For example:

pos + frame_size + csize > size

becomes

csize > size - frame_size - pos

(but note, this assumes that you’ve already checked (size >= frame_size + pos) and that (frame_size + pos) won’t overflow)

---

The actual error Coverity is giving in this case is “Using tainted variable size as a loop boundary” (if you ‘show details’). I think the way to satisfy Coverity (and, in any case, the right thing to do) is to check both the lower and upper bound of all the tainted variables; and do it as early as possible so the taint doesn’t propagate.