April 2009 - Dillo-dev - lists.mailman3.com

[Patch 2/4] Refactoring src/prefs.c and src/dir.c

by jcid＠dillo.org

Hi Tim, On Tue, Apr 21, 2009 at 03:48:49PM +0000, Tim Nieradzik wrote: > > The patch has a strange comment WRT equal signs: > > > > + // parse the value > > + i = 0; > > + while (*p) { > > + // skip all spaces and equal signs > > + if (!isspace(*p)) { > > + value[i++] = *p; > > + } > > + > > + p++; > > + } > > + > > + value[i] = '\0'; > > > > So either the implementation or the comment is wrong... > > The comment is wrong, I am sorry. If you look a few lines above, you > will see that we have already checked whether the current character > is an equal sign. > > In a previous implementation I used to have an additional check > there but removed it later as we do not want to skip any equal signs > within values. Equal signs are very common in search URLs so > skipping them could have led to many problems. > > I guess I just forgot to adjust the comment accordingly after having > changed this part. > > > If the required job is basically the same, I can keep the current > > function (maybe returning the new termination codes) and continue > > integrating the patchset. > > Yes, that is probably best. OK, a new patch for this was just committed. Several changes/fixes were made. Of the top of my head: - Remove multiple return points inside functions - Comments accuracy - Keep source code file margin < 80 cols. - Identation style in dillo uses 3 spaces. (and there was wrong indentation too). - FIxed a memory leak. Tim: please make a diff between a tree with the original patch and what was committed to get a detailed view of the changes. I'll continue integrating from here. -- Cheers Jorge.-

April 28, 2009

towards a dillo-2.1 release

by jcid＠dillo.org

On Mon, Apr 27, 2009 at 09:10:03PM +0000, corvid wrote: > Jorge wrote: > > On Mon, Apr 20, 2009 at 10:46:49PM +0000, corvid wrote: > > > The dicache changes broke scaled multiscan images. > > > I don't think FltkImgbuf::newScan() gets called anymore. > > > > You're right. Can you look at it? > > > > BTW: FltkImgbuf::newScan() can be issued by adding a call to > > a_Image_new_scan(). > > I think this is all that's needed: > Committed. -- Cheers Jorge.-

April 28, 2009

Image callback race condition

by onepoint＠starurchin.org

On Sun, Apr 26, 2009 at 11:23:07AM +0200, Hofmann Johannes wrote: > On Sun, Apr 26, 2009 at 08:56:15AM +0100, Jeremy Henty wrote: > > > > I am logging the creation and destruction of DilloImage structures > > (patches attached) and I have confirmed that sometimes the image > > callbacks run after the Image is freed. > > This might make sense: > > DilloJpeg sometimes outlives the corresponding DilloImage. If the > a_Jpeg_callback is called and it accesses the pointer to DilloImage > we get the invalid reads, and if there is no callback anymore we > have the leak. OK, I'll try tracing the Dillo{Png,Jpeg,Gif} structures too. Regards, Jeremy Henty

April 28, 2009

towards a dillo-2.1 release

by corvid＠lavabit.com

Jorge wrote: > On Mon, Apr 20, 2009 at 10:46:49PM +0000, corvid wrote: > > The dicache changes broke scaled multiscan images. > > I don't think FltkImgbuf::newScan() gets called anymore. > > You're right. Can you look at it? > > BTW: FltkImgbuf::newScan() can be issued by adding a call to > a_Image_new_scan(). I think this is all that's needed:

April 27, 2009

Image callback race condition

by Johannes.Hofmann＠gmx.de

On Sun, Apr 26, 2009 at 08:56:15AM +0100, Jeremy Henty wrote: > > I am logging the creation and destruction of DilloImage structures > (patches attached) and I have confirmed that sometimes the image > callbacks run after the Image is freed. Here's a typical example: > > $ grep '^Image: 0x4687db8: ' ../../Log/log_2009-04-25_21-48-48.txt > Image: 0x4687db8: new > Image: 0x4687db8: ref: a_Jpeg_callback > Image: 0x4687db8: free > Image: 0x4687db8: ref: a_Jpeg_callback > Image: 0x4687db8: ref: a_Jpeg_callback This might make sense: DilloJpeg sometimes outlives the corresponding DilloImage. If the a_Jpeg_callback is called and it accesses the pointer to DilloImage we get the invalid reads, and if there is no callback anymore we have the leak. Cheers, Johannes

April 26, 2009

Image callback race condition

by onepoint＠starurchin.org

I am logging the creation and destruction of DilloImage structures (patches attached) and I have confirmed that sometimes the image callbacks run after the Image is freed. Here's a typical example: $ grep '^Image: 0x4687db8: ' ../../Log/log_2009-04-25_21-48-48.txt Image: 0x4687db8: new Image: 0x4687db8: ref: a_Jpeg_callback Image: 0x4687db8: free Image: 0x4687db8: ref: a_Jpeg_callback Image: 0x4687db8: ref: a_Jpeg_callback So how can I track down this race condition? Regards, Jeremy Henty PS: I am still running corvid's img-ref patch (also attached for reference).

April 26, 2009

Dillo documentation online

by onepoint＠starurchin.org

There's a tarball of the Dillo documentation at http://starurchin.org/dillo/dillo-docs.tar.gz Alternatively, download the link called "Documentation" at http://starurchin.org/dillo/ It will be updated every night, although it will only be as current as my local HG repo (which I update most days). Regards, Jeremy Henty

April 25, 2009

image memory leak

by onepoint＠starurchin.org

On Fri, Apr 24, 2009 at 06:20:20PM +0200, Hofmann Johannes wrote: > On Thu, Apr 23, 2009 at 09:36:23PM +0100, Jeremy Henty wrote: > > > > On Thu, Apr 23, 2009 at 06:01:56PM +0200, Hofmann Johannes wrote: > > > > > It turns out that memory related to aborted images is not released. > > > > What about the other way round, ie. memory being released and > > then accessed? If you check the invalid read errors at the > > valgrind logs page[1] for the past few days you will see they are > > all due to reading freed image memory. Is this another aspect of > > the same thing, or something else? > > I'd say it's something different. It's always a read of size 4, > isn't it? I guess that's just one uninitialized pointer. I don't think that it's uninitialized pointers. If it were then you would expect valgrind to complain that the address had never been malloced or freed. That's not what we see. Eg. look at the first instance of 'Jpeg_write: Invalid read of size 4'[1]. The freed memory is a DilloImage structure. The line that does the invalid read is reading ... a member of a DilloImage structure! The same is true for the errors from dw::core::Widget::getLayout() , Png_datarow_callback and Png_datainfo_callback . In every case the pointer refers to a malloced structure of the right type. The problem is that the structure has since been freed. I think it's pretty clear that the problem is not uninitialised pointers, it's that the code is using pointers after freeing them. It's probably significant that these errors all come from within image callbacks (a_Jpeg_callback and a_Png_callback). It looks like a race condition to me: callbacks are running after the image has been freed. Does that make sense? Regards, Jeremy Henty [1] http://starurchin.singularity/dillo/valgrind/91efbf6a9f7430ca10f371db76030c…

April 25, 2009

image memory leak

by Johannes.Hofmann＠gmx.de

On Thu, Apr 23, 2009 at 09:36:23PM +0100, Jeremy Henty wrote: > > On Thu, Apr 23, 2009 at 06:01:56PM +0200, Hofmann Johannes wrote: > > > It turns out that memory related to aborted images is not released. > > What about the other way round, ie. memory being released and then > accessed? If you check the invalid read errors at the valgrind logs > page[1] for the past few days you will see they are all due to reading > freed image memory. Is this another aspect of the same thing, or > something else? I'd say it's something different. It's always a read of size 4, isn't it? I guess that's just one uninitialized pointer. Cheers, Johannes

April 24, 2009

image memory leak

by onepoint＠starurchin.org

On Thu, Apr 23, 2009 at 06:01:56PM +0200, Hofmann Johannes wrote: > It turns out that memory related to aborted images is not released. What about the other way round, ie. memory being released and then accessed? If you check the invalid read errors at the valgrind logs page[1] for the past few days you will see they are all due to reading freed image memory. Is this another aspect of the same thing, or something else? > I tried to figure out, why the first cache client is not getting > aborted properly but the cache related code is quite complex... > Does anyone know how this should work? No, I found that part of the code rather brainbending I'm afraid. Regards, Jeremy Henty [1] http://starurchin.org/dillo/valgrind.html

April 23, 2009