Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Quant.c ptr or value bug #52

Closed
abadger opened this Issue · 9 comments

3 participants

@abadger

This commit seems to contain a couple bugs fluggo@4459715

However: (1) I think the bug was already present

(2) I've been wandering around the C code for several hours and I'm not sure whether the bug should be fixed by using a pointer type or by converting from a pointer to an int.

Let me walk you through what I'm seeing and maybe you or fluggo can figure out what needs to be done:

On a 64bit *nix system (Running on x86_64, Fedora Linux):

$ gcc -Wall python2.7-config --includes -Wall -c Quant.c
Quant.c: In function 'rehash_collide':
Quant.c:154:23: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
Quant.c:154:39: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
Quant.c:154:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
Quant.c: In function 'hash_to_list':
Quant.c:247:14: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]

Looking at the code for those we have:

rehash_collide(HashTable h, void **keyp, void **valp, void *newkey, void *newval) {
    *valp = (void *)(((int) *valp) + ((int) newval));
}

and (heavily cut for relevancy):

static void
hash_to_list(HashTable h, const void *key, const void *val, void *u) {
   PixelList *p;
   int count=(int) val;
   p->count=count;

On 64 bit Linux where Pointer size is equal to the sizeof Long, those are both wrong as they are trying to fit a pointer into a int sized variable. If that's the extent of the bug, one solution would be to use a typedef that's been sized appropriately for the platform you're on. For instance:

#include<stdint.h>

intptr_t count=(intptr_t) val;

Note that you also have to modify the places where that value is then stored permanently. For instance, PixelList->count needs to change from an (int) to an (intptr_t).

However, I'm not certain that this is the extent of the bug. Looking at more of the code it seems strange to me that PixelList->count is an int type when we seem to be storing the address of the val variable above. I also see in the splitLists() function that we're adding PixelList->counts together which wouldn't make sense if they're all addresses. Perhaps Does there need to be a dereference in there like this?

int count=*( (int *)val );

For rehash_collide(), I couldn't definitely figure out what the code was doing but I vaguely feel that it's making a similar mistake. valp = (void *)(((int) *valp) + ((int) newval)); seems like it's adding two pointer addresses together which doesn't seem like it would result in anything meaningful. Perhaps the code is supposed to add the values being pointed at together? So dereference valp twice and dereference newval once, add the results and then store them into *valp:

**(int **)valp = **((int **)valp) + *(int *)(newval);

Anyhow, there's definitely a bug here but since I don't definitely know if we're actually trying to store pointer addresses or integer values, I'm not sure which of these paths is the right one to follow to correct it.

@manisandro

I'm also rather clueless about how this code should work (all that unsigned longs getting casted to void* code looks terribly fragile...). The good news, however, is that the quantize function was apparently deprecated some time ago. The PIL 1.1.6 documentation states

quantize #
im.quantize(colors, **options) ⇒ image
(Deprecated) Converts an “L” or “RGB” image to a “P” image with the given number of colors, and returns the new image.
For new code, use convert with a adaptive palette instead:
out = im.convert("P", palette=Image.ADAPTIVE, colors=256)

And the PIL 1.1.7 (and Pillow) documentations do not even mention the function anymore. I would therefore suggest to simply remove the quantization code.

@aclark4life
Owner

Is there anyone else we could get to look at this before we start removing code?

@manisandro

I might add: since the hash table values are unsigned longs casted to void*, the code section in question should probably look like

*valp=(void *)(((unsigned long)valp)+((unsigned long)&newval));

and similarly for many other places where val gets casted to an int.

I could make a pull request with the corresponding changes.

@aclark4life
Owner

If it's not too much trouble, please do. Then post to image-sig and ask for code reviews (assuming no one else jumps in here).

@manisandro manisandro referenced this issue
Merged

Fix issue #52 #59

@manisandro

A follow up on this: What the author of the code wanted to do is to implement a generic hash table structure, and since c does not have c++ templates, he just used void* as a generic data holder and casted to and from void* every time the actual value was needed. This is not at all robust concerning portability, so a better fix would be to work with typedefs for the hash table key and value and remove this fake templateization. But that would be a fairly big effort, and question is whether it's worth it since the function is deprecated anyway. Anyhow, the commit in the pull request should fix the issue for 32 and 64 bit systems, where the size of the pointer is equal to unsigned long.

@manisandro

Any news on this?

@aclark4life
Owner

I can merge it if it's time to do so, i.e. if it has been properly reviewed and there are no controversies

@manisandro

I ran im.quantize on some random images, comparing the patched Pillow with PIL 1.1.7 (on linux 32 and 64), and the results are identical (even the binary contents) between the two version - and the output the expected one. So I'd say it's safe to merge. As I stated above, the design with the void*-to-int conversion is not really optimal, but the patch for sure does not make it any worse.

@aclark4life
Owner

Works for me, thanks!

@aclark4life aclark4life closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.