2012.02.28 19:56 "[Tiff] Confusion in implementing _TIFFVGetField, adding a custom TAG", by BigEd781

2012.02.28 22:02 "Re: [Tiff] Confusion in implementing _TIFFVGetField, adding a custom TAG", by BigEd781

So I just noticed one fact; the YCBR example that I site actually uses an

array of two int16's and the argument passed to the va_list is an int32.

Under most implementations this would be fine as the va_list would only be

incremented by the size of an int16, so you would not get read/write errors

(data could be bad, but I assume the implementer knew how to handle this).

I'm new to libtiff, so I want to make sure that my assumptions are correct, but I believe that I have stumbled across an obvious issue in the library.

I am adding my own built-in tags to the library using the documentation here: http://libtiff.maptools.org/addingtags.html

So, I added an entry to the TIFFFieldInfo array defined at the top of tif_dirinfo.c like so:

{ TIFFTAG_CUSTOM_XXX, 4, 4, TIFF_SLONG, FIELD_XXX,
1, 0, "XXX" },

I then added a field to the TIFFDirectory structure defined in tif_dir.h:

int32 td_xxx[4];

Now I went ahead and modified _TIFFVSetField and _TIFFVGetField as instructed. This is where I hit a problem.

In mimicking the pattern already present in the library, I added the following code to _TIFFVGetField:

> case TIFFTAG_CUSTOM_XXX:

 *va_arg(ap, int32*) = td->td_xxx[0];
        *va_arg(ap, int32*) = td->td_xxx[1];
        *va_arg(ap, int32*) = td->td_xxx[2];
        *va_arg(ap, int32*) = td->td_xxx[3];

        break;

As far as I can tell this is completely incorrect. For reference please look to the section of the switch which deals with TIFFTAG_YCBCRSUBSAMPLING. This is analogous to what I am doing, i.e., populate the input based off of an array of ints, the only difference being the size of the data type.

_TIFFVGetField is eventually called from TIFFWriteNormalTag in tif_dirwrite.c. The relevant code is this:

case TIFF_LONG:
        case TIFF_SLONG:
        case TIFF_IFD:
                if (fip->field_passcount) {
                        uint32* lp;
                        if (wc == (uint16) TIFF_VARIABLE2) {
                                TIFFGetField(tif, fip->field_tag, &wc2, &lp);
                                TDIRSetEntryCount(tif,dir, wc2);
                        } else { /* Assume TIFF_VARIABLE */
                                TIFFGetField(tif, fip->field_tag, &wc, &lp);
                                TDIRSetEntryCount(tif,dir, wc);
                        }
                        if (!TIFFWriteLongArray(tif, dir, lp))
                                return 0;
                } else {
                        if (wc == 1) {
                                uint32 wp;
                                /* XXX handle LONG->SHORT conversion */
                                TIFFGetField(tif, fip->field_tag, &wp);
                                TDIRSetEntryOff(tif,dir, wp);
                        } else {
/* ----------------------------------------------- */
/* this is the code that is called when I write my file */
                                uint32* lp;
                                TIFFGetField(tif, fip->field_tag, &lp);
                                if (!TIFFWriteLongArray(tif, dir, lp))
                                        return 0;
                        }
                }
                break;

So, an uninitialized pointer lp is declared and its address passed to TIFFGetField. This in turn sets up the va_list and calls TIFFVGetField, which calls _TIFFVGetField with the supplied va_list and the pointer to the unitialized pointer.

There are two problems here.

1. This is how the library extracts the data (my code, but again, following the pattern already present)

*va_arg(ap, int32*) = td->td_xxx[0];

This seems incorrect. It is setting the original pointer to the value of an int. I reasoned that perhaps, in the example I was following (TIFFTAG_YCBCRSUBSAMPLING), these integers were actually addresses. even if that's the case though there is another problem.

The next problem is that we call va_args N times, where N is the number of elements in the array. From what I see the variable argument list only ever contains a single argument (the address of a pointer). This is undefined behavior per the standard (important bit at the begining):

[b]If there is no actual next argument[/b], or if type is not compatible with the type of

the actual next argument (as promoted according to the default argument promotions), the behavior is undefined.

The correct version would be

*va_arg(ap, int32**) = td_xxx;

This sets the previously uninitialized pointer to the array, which is valid. I don't like that it points to the data itself instead of a copy, but whatever, at least it doesn't crash and gives me the correct result.

My concern here is that I am missing something subtle. This software is old and used by many, many people. As such, calling this a bug feels to me like blaming a crash on the compiler, which is *almost* always wrong. However, I cannot reason out a way in which this is correct, specifically how the library writes to what va_arg returns when called more than one time.

Any help would be greatly appreciated. Thanks in advance.

--

View this message in context: http://old.nabble.com/Confusion-in-implementing-_TIFFVGetField%2C-adding-a-custom-TAG-tp33409332p33410037.html

Sent from the Tiff / LibTiff mailing list archive at Nabble.com.