Created attachment 276779 [details] [review]
addressbook: Add bounds checking to a summary file function

There was not a bug here, as the len parameter always came from a
guint16, so no overflow was possible. However, a bounds check is always
a good idea and makes the code more robust in the face of future
changes.

Coverity issue: #1061517

Review of attachment 276779 [details] [review]:

Heh, G_MAXSIZE is quite large value. Feel free to commit. Thanks.

Attachment 276779 [details] pushed as 2a5d6db - addressbook: Add bounds checking to a summary file function

Created attachment 289779 [details] [review]
addressbook: Fix ordering of parameters to fread()

This is a follow up to commit 2a5d6db6, since it did not eliminate the
use of tainted data in the sense that fread() was being called with a
variable-sized size argument, when it should be called with a
variable-sized nmemb argument.

Coverity issue: #1061517

Created attachment 289780 [details] [review]
camel: Fix ordering of parameters to fread()

fread() takes two integer parameters: size and nmemb. size should always
be constant, as it is a structure size. nmemb may vary. Using these two
parameters the correct way around means the return value is consistently
related to nmemb, and static analysers such as Coverity can perform
taint tests on the values passed to size.

Coverity issue: #1061517

So, having rescanned with Coverity, this didn’t entirely fix the issue. Two patches attached to fix it further, and the same underlying issue in other places in EDS.

Review of attachment 289779 [details] [review]:

Looks good, feel free to commit to sources.

::: addressbook/libedata-book/e-book-backend-summary.c
@@ +275,3 @@
 {
 	gchar *buf;
+	gsize rv;

it's not accurate, if size_t changes (fread's return value is size_t), but I guess this will work too, thus feel free to commit it.

Review of attachment 289780 [details] [review]:

::: camel/providers/nntp/camel-nntp-store-summary.c
@@ +296,3 @@
 	}
 
+	if (fread (is->last_newslist, NNTP_DATE_SIZE, 1, in) != 1)

This change doesn't make much sense to me. At all the other occurrences (attached patches here) you changed the call in the other way, that the chunk size is 1 byte (sizeof(gchar)), and you save 'len' chunks, while you expect to have read/written 'len' chunks. Why do you do that here in the opposite way?

Created attachment 289911 [details] [review]
camel: Fix ordering of parameters to fread()

fread() takes two integer parameters: size and nmemb. size should always
be constant, as it is a structure size. nmemb may vary. Using these two
parameters the correct way around means the return value is consistently
related to nmemb, and static analysers such as Coverity can perform
taint tests on the values passed to size.

Coverity issue: #1061517

(In reply to comment #8)
> This change doesn't make much sense to me. At all the other occurrences
> (attached patches here) you changed the call in the other way, that the chunk
> size is 1 byte (sizeof(gchar)), and you save 'len' chunks, while you expect to
> have read/written 'len' chunks. Why do you do that here in the opposite way?

You’re right. I misread NNTP_DATE_SIZE as being something like sizeof(NntpDate), rather than being the length of a string. I’ve reverted that patch chunk.

Comment on attachment 289779 [details] [review]
addressbook: Fix ordering of parameters to fread()

Committed the first patch with the s/gsize/size_t/ change.

Attachment 289779 [details] pushed as 9ccd4ce - addressbook: Fix ordering of parameters to fread()

Created attachment 289916 [details] [review]
camel: Fix ordering of parameters to fwrite()

fwrite() takes two integer parameters: size and nmemb. size should always
be constant, as it is a structure size. nmemb may vary. Using these two
parameters the correct way around means the return value is consistently
related to nmemb, and static analysers such as Coverity can perform
taint tests on the values passed to size.

Created attachment 289917 [details] [review]
addressbook: Fix ordering of parameters to fwrite()

fwrite() takes two integer parameters: size and nmemb. size should always
be constant, as it is a structure size. nmemb may vary. Using these two
parameters the correct way around means the return value is consistently
related to nmemb, and static analysers such as Coverity can perform
taint tests on the values passed to size.

Review of attachment 289911 [details] [review]:

Looks good, feel free to commit to sources.

Review of attachment 289916 [details] [review]:

Looks good, feel free to commit to sources.

Review of attachment 289917 [details] [review]:

Looks good, feel free to commit to sources.

Attachment 289911 [details] pushed as 6a69786 - camel: Fix ordering of parameters to fread()
Attachment 289916 [details] pushed as f81b1ec - camel: Fix ordering of parameters to fwrite()
Attachment 289917 [details] pushed as 08b65f5 - addressbook: Fix ordering of parameters to fwrite()