Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix some segfaults #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix some segfaults #165

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2014

This commit has been contributed by someone who despises github's
force-https behaviour and who didn't find upstream email contact
to send a patch; feel free to adapt it as you like, copyright is
assigned to upstream.

Here's a reference to quite tangential posting in Russian regarding
a completely different discussion that resulted in me proposing
to proxy the patches considered worthwile (I know that person online
for some time and he seems knowledgeable albeit rough):

http://www.opennet.ru/openforum/vsluhforumID3/93380.html#172

HTH

This commit has been contributed by someone who despises github's
force-https behaviour and who didn't find upstream email contact
to send a patch; feel free to adapt it as you like, copyright is
assigned to upstream.

Here's a reference to quite tangential posting in Russian regarding
a completely different discussion that resulted in me proposing
to proxy the patches considered worthwile (I know that person online
for some time and he seems knowledgeable albeit rough):

http://www.opennet.ru/openforum/vsluhforumID3/93380.html#172

HTH
@richardgv
Copy link
Collaborator

Thanks for the patch! But before merging it, I would prefer to know the exact steps to reproduce the segfault. We have not received similar reports so far.

... who didn't find upstream email contact to send a patch ...

There isn't an official one as far as I know. Patches could be sent to our email addresses, which could be found in git history.

I don't quite understand why GitHub's force HTTP behavior is bad, though -- imagine what would happen if an contributor of the most popular repo (I suppose, Linus Torvalds) gets his cookies leaked because he accidentally accessed GitHub with plain HTTP in an insecure environment.

@ghost
Copy link
Author

ghost commented Jan 6, 2014

On Mon, Jan 06, 2014 at 12:10:16AM -0800, Richard Grenville wrote:

Thanks for the patch! But before merging it, I would prefer to
know the exact steps to reproduce the segfault. We have not
received similar reports so far.

... who didn't find upstream email contact to send a patch ...

There isn't an official one as far as I know. Patches could be
sent to our email addresses, which could be found in git
history.

Thank you, I've forwarded this notification back to the patch author.
Hope he provides some clarification regarding segfault conditions.

I don't quite understand why GitHub's force HTTP behavior is
bad, though -- imagine what would happen if an contributor of
the most popular repo (I suppose, Linus Torvalds) gets his
cookies leaked because he accidentally accessed GitHub with
plain HTTP in an insecure environment.

Well various people have various showstoppers...

---- WBR, Michael Shigorin / http://altlinux.org
------ http://opennet.ru / http://anna-news.info

@ghost
Copy link
Author

ghost commented Jan 6, 2014

Proxying back the answer... Richard, thanks for reply, I've bounced you the original email with much smaller patch attached (looks like the author's email address is intentionally public anyways); here's its copy for inspection (or should I prepare a separate branch with a separate pull request?): http://fly.osdn.org.ua/~mike/tmp/sf8.patch -- and here's a copy of that message's body:


Thanks for the patch! But before merging it, I would prefer to know
the exact steps to reproduce the segfault. We have not received
similar reports so far.
ah. there are no exact steps to reproduce segfault. i catched it 'cause
compton segfaults once in a while (once per hour, or once per day...).
then i started compton under valgrind and found that it (compton)
accesses free()d memory. it's just a matter of luck to have the
necessary data untouched there.

one of the things that makes compton crash faster was to quickly open
and close alot of windows with transition effects of various speed (i
don't remember the exact setting that crashes often, sorry). not just
mapping/unmapping, but real destroying.

but the easiest way (as i said) is to run compton with valgrind. you'll
found some more bugs too (i was too lazy to investigate that cases
further).

btw, the previous patch reverts some recent git commits, so a attached
another one made against 4868d1e.

... who didn't find upstream email contact to send a patch ...
There isn't an official one as far as I know. Patches could be sent
to our email addresses, which could be found in git history.
i used to think that don't having 'official' e-mail in README means
"we don't want to take any patches via e-mail".

I don't quite understand why GitHub's force HTTP behavior is bad,
though -- imagine what would happen if an contributor of the most
popular repo (I suppose, Linus Torvalds) gets his cookies leaked
because he accidentally accessed GitHub with plain HTTP in an
insecure environment.

  1. nothing harmfull. there is no way to modify actual repository files
    with webface.
  2. forcing https makes me feel like a dumbass which should be guided
    thru any obvious action. i skilled enough to decide for myself which
    protocol i want to use.

there is another reason with github too: wording of their letter after
'rails breakout incident'. that wasn't me who fubared yet i received an
ORDER to perform some actions to fix github's fault. i don't like to
get orders and i especially don't like to be ordered to fix things that
i never broke. so i demand personal apologies from github before i
consider to work with that team of shitheads again.

@richardgv
Copy link
Collaborator

one of the things that makes compton crash faster was to quickly open
and close alot of windows with transition effects of various speed (i
don't remember the exact setting that crashes often, sorry). not just
mapping/unmapping, but real destroying.

but the easiest way (as i said) is to run compton with valgrind. you'll
found some more bugs too (i was too lazy to investigate that cases
further).

I wrote a script that quickly spawns zenity --error then kills them at a fixed interval, and ran compton (git-v0.1_beta2-7-g4868d1e-2014-01-06) with valgrind --leak-check=full --blur-background --backend glx --glx-no-stencil --glx-no-rebind-pixmap --glx-swap-method 3 --config compton.sample.conf. Valgrind-3.9.0 indicates no memory access issues nor any memory leaks in the program, except a few false positives in nvidia-drivers libGL. Clang address (+leak) sanitizer doesn't report anything, either. Adjusting fade interval doesn't bring any changes.

In case it would be useful, I use xorg-server-1.14.5, nvidia-drivers-331.20, fvwm-2.6.5 on a Gentoo ~amd64 with kernel 3.12.3-pf. I build compton with CFG_DEV=1 (clang-3.3, ld.gold, -ggdb).

nothing harmfull. there is no way to modify actual repository files
with webface.

You could merge pull requests with the web interface, and perform administrative tasks, like adding contributor, adding SSH keys, and deleting repos.

forcing https makes me feel like a dumbass which should be guided
thru any obvious action. i skilled enough to decide for myself which
protocol i want to use.

We all make mistakes, sometimes.

@richardgv
Copy link
Collaborator

@gvy:

So apparently I failed to get the mail containing valgrind log. Could you please attach it here?

@ghost
Copy link
Author

ghost commented Jan 19, 2014

Only image attachments are accepted (doubt you would like PNG), here are the links:
http://pastebin.com/i6HyiMcy (compton.conf)
http://pastebin.com/NbujKm0B (z01.log)
and here's a copy of that email (just forwarded it anyways):


Hello.

compton.sample.conf`. Valgrind-3.9.0 indicates no memory access
issues nor any memory leaks in the program, except a few false
positives in nvidia-drivers libGL. Clang address (+leak) sanitizer
doesn't report anything, either.
the strange thing that i can't reproduce valgrind warnings anymore.
maybe the patch is out of date now. i didn't read all the changes
carefully and i made the patch somewhere in... august, may be? i won't
planned to use compton at all, so didn't bother to record the exact
date.

that's what i got from my valgrind around the time i made the patch:

==7597== Invalid read of size 4
==7597== at 0x8053681: paint_all (compton.c:???)
==7597== by 0x805CC5F: session_run (compton.c:???)
==7597== by 0x805CE0D: main (compton.c:???)
==7597== Address 0x6b0c570 is 200 bytes inside a block of size 432
free'd

and many similar hits in win_build_shadow(), win_paint_shadow() and
others called from paint_all(). and in paint_isvalid() too.

AH! i managed to reproduce this. i attached valgrind log against rev
4868d1e and my compton.conf. maybe this is 'cause i'm using xrender
backend. tech info: slackware x86, X.Org 1.2.12, proprietary NVidia
drivers, Fluxbox as WM.

still can't say how one can reliable reproduce this though. %-( it's
fairly easy on my box: i'm using Fluxbox keycombos to launch some
script which uses google translate to translation (obvious %-). i'm
using dmenu to make some selections from the script. executing keycombo
and then pressing 'enter' twice to skip 'src' and 'dest' language
selections makes valgrind cry. i'm pretty sure that this is the issue
with destroying windows in the process of 'fading in'.

i'll try to write a simple X11 program to trigger the bug.

@richardgv
Copy link
Collaborator

Thanks for the log. I'm able to reproduce the issue, but it's very hard to reproduce as seemingly it's highly timing-related. I need to repeat it for hundreds of times to reproduce once... Still hunting it, hold on.

richardgv added a commit that referenced this pull request Jan 21, 2014
- Fix a bug that w->prev_trans sometimes points to freed memory.
  Probably related to #165.

- Add some more debugging printf()-s under DEBUG_EVENTS.
@richardgv
Copy link
Collaborator

Heh, now this is a bit annoying... What I reproduced looks like a different bug, which I fixed in aeda148 (richardgv-dev branch). I can't reproduce your problem. Destroying windows in the process of 'fading in' does not trigger it here. Running echo a | dmenu and quickly canceling it or running and killing zenity rapidly with a script doesn't help, either.

I would be grateful if you could pull from richardgv-dev branch, build with CPPFLAGS='-DDEBUG_EVENTS' make -B, run it with valgrind, and send me all the output when the problem occurs.

By the way, your valgrind indicates Valgrind-3.9.0.SVN. Do you think an unstable version of valgrind could create false positives?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant