Crash while downloading dives

Lubomir I. Ivanov neolit123 at gmail.com
Fri Dec 14 12:23:38 PST 2012


On 14 December 2012 19:18, Dirk Hohndel <dirk at hohndel.org> wrote:
>
> Henrik Brautaset Aronsen <subsurface at henrik.synth.no> writes:
>
> > Henrik Brautaset Aronsen wrote:
> >> It's been awhile since I tried downloading dives with Subsurface. But I
> >> just tried it again, and the download dialogue and progress bar looks
> >> really smooth now! Kudos!
> >>
> >> Just one little thing, though: Subsurface crashed while downloading. I
> >> wasn't paying attention, but I think it must have been while as it
> >> finished downloading. Here's the stacktrace:
> >>
> >> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> >> 0 libsystem_kernel.dylib 0x00007fff90120212 __pthread_kill + 10
> >> 1 libsystem_c.dylib 0x00007fff8f5dbaf4 pthread_kill + 90
> >> 2 libsystem_c.dylib 0x00007fff8f61fe9e __abort + 159
> >> 3 libsystem_c.dylib 0x00007fff8f5e171d __chk_fail + 35
> >> 4 libsystem_c.dylib 0x00007fff8f5e1826 __snprintf_chk + 150
> >> 5 subsurface 0x0000000106df5d87 set_dc_nickname + 727 (gtk-gui.c:2095)
> >
> >
> > It looks like the line number is slightly wrong for some reason.  This
> > is the line that crashes (I had a printf("xx\n"); just after it that
> > didn't execute):
> >
> > snprintf(nickname, 69, "%s (%08x)", dive->dc.model, dive->dc.deviceid);
> >
> > I've verified that both variables are set ("Suunto HelO2" and
> > "a7c8b7da"), so I don't really understand why it fails here.
>
> Oh how odd. The code clearly has a bug, but with those two short stings
> I really wonder why it triggers a crash on MacOS. Can you change that
> line to read
>
> snprintf(nickname, sizeof(nickname), "%s (%08x)", dive->dc.model, dive->dc.deviceid);
>

i think it should be sizeof(nickname), but not (sizeof(nickname) + 1),
or it could write the \0 byte outside of the buffer and then the error
may be runtime/implementation dependent.
btw, the GTK API says that gtk_entry_set_max_length() counts in
characters, which are 2 byte UTF8.
that alone won't be a problem in this particular case.

the snprintf() call in set_dc_nickname() isn't UTF8 safe, but given
that models and deviceid's are predefined (and english only?) they
will probably always fit.
larger stack buffers (like 256 bytes) won't hurt much here.

in remember_dc() the 80 byte buffer is prone to UTF8 validation
errors, since the dialog box will allow the user to enter 68 unichars
or 136 chars without \0.
but if the input string is a mix of unichars and chars and the
resulted length ends up as an odd number somehow, snprintf()'s
truncate may split an unichar in half.
a simple solution here would be to make sure that "buffer" in
remember_dc() has at least twice the size (+ 8 (id) + k (extra chars))
of the input dialog N characters.

i'm able to trigger some validation errors (i.e. an empty string goes
into the config), but i don't really have good means to test this
given i don't have a DC, so perhaps its best not sending an actual
patch.

hope that helps.
lubomir
--


More information about the subsurface mailing list