Comment 11 for bug 750294

Revision history for this message
Peter Seiderer (ps-report) wrote :

Same Problem found with Showwell-11.5 on openSUSE 12.1 (x86_64).

Problem analysis (gdb trace/comments):

$ gdb ./shotwell
(gdb) run

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff02b5540 in __memcpy_ssse3 () from /lib64/libc.so.6
(gdb) where
#0 0x00007ffff02b5540 in __memcpy_ssse3 () from /lib64/libc.so.6
#1 0x0000000000604c9a in gp_load_file_into_buffer (context=0x1911740, camera=0x194c950, folder=0x1873270 "/DCIM/100CANON", filename=0xe33a50 "IMG_0001.JPG",
    filetype=GP_FILE_TYPE_EXIF, result_length1=0x7fffffffb1c0, error=0x7fffffffb1b8) at /home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala:248
#2 0x0000000000603c87 in gp_load_metadata (context=0x1911740, camera=0x194c950, folder=0x1873270 "/DCIM/100CANON", filename=0xe33a50 "IMG_0001.JPG", error=
    0x7fffffffb2a8) at /home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala:176
#3 0x0000000000615331 in import_page_load_previews_and_metadata (self=0x12d4570, import_list=0x142f040)
    at /home/seiderer/Work/shotwell/shotwell/src/camera/ImportPage.vala:1405
#4 0x00000000006114d9 in import_page_refresh_camera (self=0x12d4570) at /home/seiderer/Work/shotwell/shotwell/src/camera/ImportPage.vala:1125
#5 0x000000000060fa85 in import_page_try_refreshing_camera (self=0x12d4570, fail_on_locked=0)
    at /home/seiderer/Work/shotwell/shotwell/src/camera/ImportPage.vala:952
#6 0x000000000060f936 in import_page_real_switched_to (base=0x12d4570) at /home/seiderer/Work/shotwell/shotwell/src/camera/ImportPage.vala:943
...

(gdb) frame 1
#1 0x0000000000604c9a in gp_load_file_into_buffer (context=0x1911740, camera=0x194c950, folder=0x1873270 "/DCIM/100CANON", filename=0xe33a50 "IMG_0001.JPG",
    filetype=GP_FILE_TYPE_EXIF, result_length1=0x7fffffffb1c0, error=0x7fffffffb1b8) at /home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala:248
248 Memory.copy(buffer, data, buffer.length);
(gdb) p buffer
$1 = (guint8 *) 0x1955110 ""
(gdb) p data
$2 = (guint8 *) 0x0

---> data (the location from where to copy) is null

(gdb) p buffer.length
Attempt to extract a component of a value that is not a structure.

---> gdb could not show vala data types, take a look at the compiled version of GPhoto.vala,
---> a generated C file src/camera/GPhoto.c where the memcpy takes place:

2182 #line 248 "/home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala"
2183 _tmp31__length1 = buffer_length1;
2184 #line 248 "/home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala"
2185 memcpy (_tmp29_, _tmp30_, (gsize) _tmp31__length1);
2186 #line 250 "/home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala"

(gdb) p _tmp29_
$3 = (guint8 *) 0x1955110 ""
(gdb) p _tmp30_
$4 = (guint8 *) 0x0

---> _tmp30_/data is realy null (no gdb debug info failure)

(gdb) p _tmp31__length1
$5 = 21336

---> _tmp31__length1/buffer.length is set, so take a look at vala source code

(gdb) list
234
235 res = camera.get_file(folder, filename, filetype, camera_file, context);
236 if (res != Result.OK)
237 throw new GPhotoError.LIBRARY("[%d] Error retrieving file object for %s/%s: %s",
238 (int) res, folder, filename, res.as_string());
239
240 // if buffer can be loaded into memory, return a copy of that (can't return buffer itself
241 // as it will be destroyed when the camera_file is unref'd)
242 unowned uint8[] data;
243 res = camera_file.get_data_and_size(out data);
244 if (res != Result.OK)
245 return null;
246
247 uint8[] buffer = new uint8[data.length];
248 Memory.copy(buffer, data, buffer.length);
249
250 return buffer;

---> camera_file.get_data_and_size(out data) return code is checked, but data is null?
---> one more look at the generated C file

2123 #line 243 "/home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala"
2124 _tmp21_ = camera_file;
2125 #line 243 "/home/seiderer/Work/shotwell/shotwell/src/camera/GPhoto.vala"
2126 _tmp24_ = gp_file_get_data_and_size (_tmp21_, &_tmp22_, &_tmp23_);

---> check the parameters for the gp_file_get_data_and_size(CameraFile*, const char **data, unsigned long int *size) call

(gdb) p _tmp24_
$6 = 0

---> return value is o.k.

(gdb) p _tmp22_
$7 = (guint8 *) 0x0

---> data is set to null

(gdb) p _tmp23_
$8 = 21336

---> size seems to be o.k.
---> what now? check the data types...

(gdb) ptype _tmp22_
type = unsigned char *

---> o.k.

(gdb) ptype _tmp23_
type = int

---> here it is what causes the bug, on x86_64 the type 'int' is 4 bytes long
---> and the type 'unsigned long' is 8 bytes

(gdb) p sizeof(int)
$9 = 4
(gdb) p sizeof(unsigned long)
$10 = 8

---> take a look at the addresse of _tmp22_ (data) and _tmp23_ (size)

(gdb) p &_tmp22_
$11 = (guint8 **) 0x7fffffffb008
(gdb) p &_tmp23_
$12 = (gint *) 0x7fffffffb004

---> what happens? From libgphoto2-2.4.11/libgphoto2/gphoto2-file.c

 350 gp_file_get_data_and_size (CameraFile *file, const char **data,
 351 unsigned long int *size)
 352 {
 353 CHECK_NULL (file);
 354
 355 switch (file->accesstype) {
 356 case GP_FILE_ACCESSTYPE_MEMORY:
 357 if (data)
 358 *data = file->data;
 359 if (size)
 360 *size = file->size;
 361 break;

---> first at line 358 file->data pointer/8 bytes are written to the adress &_tmp22_/0x7fffffffb008
---> second at line 360 file->size/8 bytes are written to the address & &_tmp23/0x7fffffffb004
---> overwriting 4 bytes of _tmp22_ (because of the type mismatch) which leads to the null pointer
---> in the memcopy

Suggested patch (fix libgphoto2/get_data_and_size() vala binding, adjust calls in src/camera/GPhoto.vala):

diff --git a/src/camera/GPhoto.vala b/src/camera/GPhoto.vala
index e2af2c8..c02a3fd 100644
--- a/src/camera/GPhoto.vala
+++ b/src/camera/GPhoto.vala
@@ -201,11 +201,12 @@ namespace GPhoto {
         // if entire file fits in memory, return a stream from that ... can't merely wrap
         // MemoryInputStream around the camera_file buffer, as that will be destroyed when the
         // function returns
- unowned uint8[] data;
- res = camera_file.get_data_and_size(out data);
+ unowned uint8 *data;
+ ulong data_len;
+ res = camera_file.get_data_and_size(out data, out data_len);
         if (res == Result.OK) {
- uint8[] buffer = new uint8[data.length];
- Memory.copy(buffer, data, data.length);
+ uint8[] buffer = new uint8[data_len];
+ Memory.copy(buffer, data, buffer.length);

             return new MemoryInputStream.from_data(buffer, on_mins_destroyed);
         }
@@ -239,12 +240,13 @@ namespace GPhoto {

         // if buffer can be loaded into memory, return a copy of that (can't return buffer itself
         // as it will be destroyed when the camera_file is unref'd)
- unowned uint8[] data;
- res = camera_file.get_data_and_size(out data);
+ unowned uint8 *data;
+ ulong data_len;
+ res = camera_file.get_data_and_size(out data, out data_len);
         if (res != Result.OK)
             return null;

- uint8[] buffer = new uint8[data.length];
+ uint8[] buffer = new uint8[data_len];