Various checks on malloc

[wiredfool]

I'd like feedback on this.

i think this is a good approach -- it follows the other changes that we've made to check for overflow on the mallocs. The biggest change is switching to calloc(n,size) instead of malloc(size) in some cases -- calloc does the multiplication, (is supposed to) check for overflow, and zeros the memory. I don't think that the zeroing is ever a bad thing, and there are some reports of the performance of calloc zeroing the memory faster than the memset that we use in a few places.

i will probably rebase/squash this prior to merging.

[coveralls]

Coverage decreased (-0.03%) to 76.852% when pulling 0239830 on wiredfool:malloc_check into 34c80ef on python-pillow:master.

[coveralls]

Changes Unknown when pulling 0564fb8 on wiredfool:malloc_check into * on python-pillow:master*.

[hugovk]

Should i also be Py_ssize_t?

[wiredfool]

No, n is set to constant values well within the range of int in different locations in this function, and i counts up to that. The reason that n is a Py_ssize_t is that it's address is passed as an argument to getlist, which needs it to be a Py_ssize_t to match the result type of the python get sequence length function.

[hugovk]

👍 calloc rather than malloc sounds sensible.

[wiredfool]

undone -

• rebase
• check coverage to make sure we're hitting the checks.

[wiredfool]

@homm Can you take a look at this PR and comment?

[homm]

Ok. I will in couple of days.

[homm]

In general, I'm against replacing malloc with calloc everywhere just for the overflow check. calloc not only does the check, it also guarantees that memory is filled with zeroes, and sometimes this can be slower.

I will take a close look at every case later.

[wiredfool]

My argument for calloc is:

1. It is safer WRT overflow.
2. It eliminates an entire class of error with uninitialized memory.
3. It's not significantly slower, at least tests aren't taking a significantly different time.

I think that the only possible drawback is performance, but from my reading elsewhere calloc is likely faster than malloc + memset, and not significantly slower than malloc due to tricks that can be played with page mapping.

[wiredfool]

Also, when looking at this, I think there's a significant speed gain to be made for larger images by combining Storage.c:ImagingNewBlock and ImagingNewArray so that we're always allocating blocks rather than switching from one block at <16M and line by line for images >16M. For example, in the current layout if you had a 2047x2048 RGBA image, that would be one big allocation of ~16M. a 2049x2048 RGBA would be 2049 allocations of 8K each.

I think it would be relatively transparent to the rest of the code if: char *im->block became char ** im->blocks. We'd add a count of blocks in the struct, and fill the im->image as normal.

[homm]

so that we're always allocating blocks

Thinking about this long time, haven't implemented yet :)

You are right that it should be transparent for most of C code except the code which directly uses ImagingNewBlock (in map.c).

[homm]

I haven't researched, but my thought are optimal block size can be about:

block_size = 2 * 2**20
mem_size = ((block_size + line_size - 1) // line_size) * line_size

[wiredfool]

Looks like the only call to it is through the _imaging.c new_block interface, from ImageTk.py. Which bypasses the threshold check to ensure that they get one big chunk of memory to pass to tk.

So the comment in ImagingNewBlock is wrong, and we should probably be returning an error where we're returning NULL from the overflow check. And ImagingDelete might need to be called on im to prevent a leak.

[homm]

why?

[wiredfool]

Inadvertent

[homm]

I've stopped in the middle. It's too large for one pass.

[wiredfool]

No worries.

I've done a quick benchmark on malloc vs calloc, where I opened and loaded a jpeg 100x, one of them was over the block limit and one was under. The difference between this branch (calloc) and master (malloc) is essentially lost in the noise. I'm seeing variances from run to run of ~1% with no clear pattern of one being faster. Calloc seems faster on the larger image, malloc on the smaller, but still pretty well within the noise level.

[wiredfool]

Comments addressed.

[homm]

Ok, I finished with the rest of changes.