[Kde-imaging] Fwd: Re: nasty bug in libkexif

[Angelo Naselli]

Hi,
this effect is probably not so present in linux,
but it can happen and make people loosing data,
as reported by Marc in OpenBSD.

I believe we should plan another libkexif with this
fix. Unfortunately I can't do it before next
weekend I believe.

If there are no volonteers, but you want to patch 
distro packages in attach a patch is present.

Regards,
   Angelo  

----------  Messaggio inoltrato  ----------

Subject: Re: nasty bug in libkexif
Date: 22:45, martedì 17 ottobre 2006
From: Marc Espie <espie at nerim.net>
To: Angelo Naselli <anaselli at linux.it>
Cc: Gilles Caulier <caulier.gilles at free.fr>

On Tue, Oct 17, 2006 at 10:26:44PM +0200, Angelo Naselli wrote:
> Alle 21:53, martedì 17 ottobre 2006, Marc Espie ha scritto:
> > On Tue, Oct 17, 2006 at 09:33:51PM +0200, Gilles Caulier wrote:
> > > Thanks Marcs for this report.
> > > 
> > > I forward your mail to Angelo to include this fix in TODO list.
> > > 
> > > Note : digikam and kipi-plugins from KDE svn trunk do use libkexif naymore. We 
> > > use Exiv2 library instead (more powerfull)
> > > 
> > > This is want mean than for next final release of digiKam (0.9.0) and 
> > > kipi-plugins (0.1.3), your bug will disapears...
> > > 
> > > Angelo, fell free to fix this little bug in svn. Thanks in advance
> > 
> > I investigated a bit more, my fopen(3) library explicitly says:
> > 
> >      Reads and writes cannot be arbitrarily intermixed on read/write streams.
> >      ANSI C requires that a file positioning function intervene between output
> >      and input, unless an input operation encounters end-of-file.
> Well as far as I can read into the same manual under linux:
>  
>        Reads and writes may be intermixed on read/write streams in any  order.
>        Note  that  ANSI  C requires that a file positioning function intervene
>        between output and input, unless an input operation encounters  end-of-
>        file.   (If this condition is not met, then a read is allowed to return
>        the result of writes other than the most recent.)  Therefore it is good
>        practice (and indeed sometimes necessary under Linux) to put an fseek()
>        or fgetpos() operation between write and  read  operations  on  such  a
>        stream.   This operation may be an apparent no-op (as in fseek(..., 0L,
>        SEEK_CUR) called for its synchronizing side effect.
> 
> The problem should be present also here. 

This manual page says the linux implementation will most often allow you
to intermix read and write without effect.

> > And Qt's QDataStream, using a QFile as device in non-raw mode, does mix
> > fread() and fwrite() without any kind of seek()'ing, so indeed, this will
> > not work on some C implementations, at least.
> Frankly speaking I don't know qt implementation and not so well kexif either.
> So Gilles what do you suggest? And which is the bad thing that can be happen
> with the implementation we have by now?
> I mean we can loose data? Security issue? 

yes, you can lose data. I found this issue by tracking a bug in digikam 
where I was ending up in corrupted images on OpenBSD.

I managed to get the problem down to this code:

#include <libkexif/kexifdata.h>
#include <libkexif/kexifutils.h>
#include <stdlib.h>

int
main(int argc, char *argv[])
{
        if (argc < 2) {
                exit(1);
        }
        KExifUtils::writeOrientation(argv[1], KExifData::NORMAL);
        exit(0);
}

under OpenBSD, this code reliably corrupts jpeg images with exif data.

In one image, the orientation marker was near the beginning of the
file, at offset ~100, but libkexif did overwrite it at offset 16384 instead,
erasing two unrelated bytes (the size of the file buffer, coincidentally).

The following patch makes sure stuff get synch'ed properly. I've tested that
it fixes the issue.

--- libkexif/kexifutils.cpp.orig	Tue Sep  6 19:30:06 2005
+++ libkexif/kexifutils.cpp	Tue Oct 17 21:32:35 2006
@@ -38,6 +38,11 @@ extern "C"
 #include "kexifutils.h"
 #include "kexifdata.h"
 
+static void streamsync(QDataStream &s)
+{
+	s.device()->at(s.device()->at());
+}
+
 bool KExifUtils::writeOrientation(const QString& filename, KExifData::ImageOrientation orientation)
 {
    QString str = "";
@@ -217,6 +222,7 @@ bool KExifUtils::writeFile(const QString
           position++;
        }
 
+       streamsync(stream);
        // write character code ASCII into offset position
        stream << 0x49435341;
        stream << 0x00000049;
@@ -260,6 +266,7 @@ bool KExifUtils::writeFile(const QString
               if(i > 8  && buf[i]==0x01 && buf[i-1]==0x00 && buf[i-2]==0x00 && buf[i-3]==0x00 
                       && buf[i-4]==0x03 && buf[i-5]==0x00 && buf[i-6]==0x12 && buf[i-7]==0x01)
               {
+	          streamsync(stream);
                   stream << (Q_UINT8)0x00;
                   stream << (Q_UINT8)orientation;
 
@@ -270,6 +277,7 @@ bool KExifUtils::writeFile(const QString
               if(i > 8 && buf[i]==0x00 && buf[i-1]==0x00 && buf[i-2]==0x00 && buf[i-3]==0x01 
                      && buf[i-4]==0x00 && buf[i-5]==0x03 && buf[i-6]==0x01 && buf[i-7]==0x12)
               {
+	          streamsync(stream);
                   stream << (Q_UINT8)orientation;
                   stream << (Q_UINT8)0x00;
 


-------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kde-imaging/attachments/20061017/e75aafd1/attachment.pgp