#66 GifDrawBoxedText8x8() modifying constant input parameter

[Igor Levicki]

Function GifDrawBoxedText8x8() promises that its input parameter legend won't be changed but then casts const char to char and does strtok() on it (which of course changes the string by putting null characters in it.

[Igor Levicki]

Hello Eric.

I think that this:

dup = malloc(strlen(legend)+1);
/ FIXME: should return bad status, but that would require API change /
if (dup != NULL) {
int i = 0;
(void)strcpy(dup, (char *)legend);

Might not be the best way to fix the problem.

First, it is not clear why use malloc() + strcpy() instead of strdup()?

Second, should the image be modified at all if this allocation fails? I personally think it should not for at least two reasons:

1. Why waste time on drawing the rectangle if the text rendering is going to fail?

2. If the text rendering is going to fail that means the legend string is malformed in which case you will surely crash in GifDrawRectangle() since it does no checking of TextWidth (w) and LineCount (d) against actual image width and height.

Also, it would be more readable if you use early exit and write:
/ compute size of text to box /
+

• dup = strdup(legend);
  +
• if (dup == NULL) {
• return;
• }
  Instead of wrapping a large piece of code in an if () block, increasing the indent for it.

Finally, in my opinion legend parameter should be passed along with its length to this function, and you should not rely on it being properly terminated. That most definitely requires either an API change or silently discarding stuff which to me seems even worse.

Regards,
Igor Levicki
igor@levicki.net

[Eric S. Raymond]

Sorry for the very belated response, it's been a busy year.

Igor Levicki igor-levicki@users.sf.net:

Hello Eric.

I think that this:

dup = malloc(strlen(legend)+1);
/ FIXME: should return bad status, but that would require API change /
if (dup != NULL) {
int i = 0;
(void)strcpy(dup, (char *)legend);

Might not be the best way to fix the problem.

First, it is not clear why use malloc() + strcpy() instead of strdup()?

Because this code runs on some extremely ancient and crufty systems that
might not have strdup(3)!

Second, should the image be modified at all if this allocation fails? I personally think it should not for at least two reasons:

1. Why waste time on drawing the rectangle if the text rendering is going to fail?

You are right. I have moved code accordingly.

    <a href="http://www.catb.org/~esr/">Eric S. Raymond</a>