| Yick | Is there anyone here works in the IT department ? | 00:00 |
|---|---|---|
| cjwatson | however it is not entirely clear to me from the comments in that bug that the segfault is in fact within isprint | 00:00 |
| penguin42 | cjwatson: Cool, so I think that bug 965341 is that it should be using iswprint | 00:00 |
| ubottu | Launchpad bug 965341 in procps (Ubuntu) "watch command line utility crashes with segfault when processing binary output" [Medium,Triaged] https://launchpad.net/bugs/965341 | 00:00 |
| penguin42 | cjwatson: comment #3 from me ? | 00:00 |
| cjwatson | I don't see how isprint could be getting inlined into main | 00:00 |
| cjwatson | that comment shows the segfault in main, afaics | 00:01 |
| penguin42 | yeh that's what the backtrace is showing - I'd thought the isprint and friends were macros or the like | 00:01 |
| juliohm | i'm also curious how penguin42 inferred about isprint() from the actual bug. | 00:01 |
| cjwatson | they might be implemented as macros, true | 00:02 |
| juliohm | oh, strace :-) | 00:02 |
| cjwatson | juliohm: he correlated gdb's output against the source code | 00:02 |
| juliohm | oh, not strace, gdb output, nice. | 00:02 |
| cjwatson | yeah, isprint is a macro that expands to an array lookup | 00:03 |
| cjwatson | if the value is out of bounds then it might well segfault | 00:03 |
| penguin42 | cjwatson: Yeh, see ctype.h - it's a macro to __isprint that ends up indexing __ctype_b_loc that's 384 bytes long ish | 00:03 |
| Yick | Is there anyone here works in the IT department ? | 00:03 |
| Yick | Is there anyone here works in the IT department ? | 00:03 |
| cjwatson | not necessarily iswprint, it might also be correct to simply check that c is representable as unsigned char before passing it to isprint | 00:03 |
| cjwatson | Yick: please don't repeat without adding further context. Why do you want such a person? | 00:04 |
| cjwatson | (whose IT department?) | 00:04 |
| Yick | any IT department. Want to know anyone has connection to Microsoft. | 00:04 |
| penguin42 | cjwatson: However I'm a bit uncomfortable saying it just needs a change from isprint to iswprint since I don't understand the wide character stuff it's wrangling with; it does have a && c<128 *after* the !isprint(c) - but I think it needs a little more understanding | 00:04 |
| * penguin42 checks the date - oh yeh | 00:05 | |
| cjwatson | penguin42: judging from the c<128 check, it isn't really interested in wide characters; I would say just !isprint(c) && c<128 => c<128 && !isprint(c) | 00:05 |
| * juliohm is curious about the details of following the execution of the watch command in gdb for backtracing the bug and fix it | 00:06 | |
| cjwatson | juliohm: it prints a line number when the program crashes, so it's pretty easy to look up from there (in this case) | 00:06 |
| cjwatson | for the rest, man gdb :-) | 00:07 |
| penguin42 | cjwatson: Yeh, the code seems to be trying to detect wide characters and set a 'carry' variable to indicate it should fetch the next byte - and I think that's probably still OK | 00:07 |
| penguin42 | juliohm: I installed the procps debug sym package, I then ran gdb `which watch` and then ran it in gdb with your args | 00:07 |
| juliohm | cjwatson, so, you have to download the source code first, right? | 00:07 |
| cjwatson | juliohm: yes, of course | 00:07 |
| penguin42 | juliohm: Then I grabbed the source (using apt-get source procps) and looked for line 541 | 00:07 |
| cjwatson | debugging programs without source code is no fun | 00:07 |
| juliohm | hmm, very interesting. | 00:08 |
| juliohm | let me try now, because maybe i can help in the future | 00:08 |
| juliohm | penguin42, what do you mean by procps debug sym package? is just the procps package? | 00:09 |
| penguin42 | juliohm: Then I looked at that line and thought there's nothing that should seg fault, printed c, noticed it was HUGE, and wondered about isprint since I'd implement it using an array | 00:09 |
| cjwatson | hmm, mind you, isprint on a wint_t is a kind of dodgy thing to do | 00:09 |
| penguin42 | juliohm: https://wiki.ubuntu.com/DebuggingProgramCrash | 00:09 |
| * juliohm is reading the link... | 00:10 | |
| penguin42 | cjwatson: Well, it does take an int (to allow EOF) | 00:10 |
| cjwatson | penguin42: sure, but technically, nothing in the standard requires int == wint_t | 00:11 |
| penguin42 | nod | 00:11 |
| cjwatson | penguin42: I think your first instinct was correct and it should be iswprint | 00:11 |
| cjwatson | assuming that tests out | 00:11 |
| penguin42 | cjwatson: What worries me is that at that point it would have the first half of a wide character and then passing that to iswprint also makes no sense | 00:12 |
| cjwatson | penguin42: how so? my_getwc appears to read a whole wide character | 00:12 |
| cjwatson | or WEOF | 00:13 |
| cjwatson | penguin42: in any case, I think the important thing is that iswprint does *not* have a range of values specified as undefined behaviour | 00:14 |
| cjwatson | oh no that's not true | 00:15 |
| cjwatson | "For all functions described in this subclause that accept an argument of type wint_t, the | 00:15 |
| cjwatson | value shall be representable as a wchar_t or shall equal the value of the macro WEOF. If | 00:15 |
| cjwatson | this argument has any other value, the behavior is undefined. | 00:15 |
| cjwatson | " | 00:15 |
| cjwatson | still, my_getwc always returns something that meets that description, I think | 00:15 |
| cjwatson | given that it always returns either rval (which is a wchar_t and gets implicitly converted) or WEOF | 00:16 |
| penguin42 | cjwatson: I'd be more comfortable if I understood what the heck that loop was actually trying to do | 00:16 |
| cjwatson | I haven't bothered to work it out but I'm confident in my statement that iswprint should never segfault in this case from dataflow analysis alone :) | 00:17 |
| cjwatson | it's some tedious display mangling, doesn't seem terribly interesting to work out the exact details | 00:18 |
| penguin42 | yeh, I'm just not sure it does what the oriignal code was supposed to | 00:18 |
| cjwatson | well, the while condition basically asks for a non-printing character under 128 that isn't newline, tab, or (sometimes) esc; so promoting to iswprint for type compatibility doesn't seem to break anything there | 00:20 |
| penguin42 | and my_getwc blows my mind - I can't see how it's while loop ever executes more than once given that it always returns in one of two ways | 00:20 |
| cjwatson | everything that manipulates c is typesafe | 00:20 |
| penguin42 | oh, the if isn't indented - ok, I see how my_getwc works | 00:21 |
| cjwatson | penguin42: it just has rotten indentation | 00:21 |
| cjwatson | yeah, the last return is conditional on 'byte == MAX_ENC_BYTES' | 00:21 |
| cjwatson | but again, I think you just need to verify that it always returns a promoted wchar_t or WEOF | 00:22 |
| cjwatson | which is pretty clear | 00:22 |
| cjwatson | it's fairly grotty code but just stick to analysing the bits you need and it's ok | 00:23 |
| penguin42 | yeh, and so my_getwc either a valid WC, a WEOF on an EOF< or WEOF on a bad char, then I think I agree iswprint should work | 00:24 |
| cjwatson | the one thing I'd note is the FORCE_8BIT bit at the top of the file, which may or may not be obsolete now | 00:24 |
| cjwatson | although leaving it there wouldn't actually hurt anything | 00:24 |
| penguin42 | yeuch | 00:26 |
| juliohm | penguin42, thank you very much for the link, it's really helpful. ;-) | 00:26 |
| juliohm | I'll try the process when i have time. | 00:26 |
| penguin42 | cjwatson: The code there comes from one of the debian patches | 00:27 |
| penguin42 | cjwatson: watch_unicode.patch | 00:27 |
| cjwatson | yep | 00:27 |
| cjwatson | is that a big deal? | 00:28 |
| penguin42 | I was just wondering if it was a debian patch or an ubuntu patch, but I think it's a debian one | 00:28 |
| cjwatson | looks like it | 00:29 |
| penguin42 | nod | 00:29 |
| penguin42 | cjwatson: Is the right thing there to add another patch to the end of the set or to fix that patch? | 00:30 |
| cjwatson | judgement call in general - in this case I'd probably fix that patch | 00:31 |
| penguin42 | ode for a 1 character tweek | 00:31 |
| cjwatson | though I wouldn't reject a branch that added another one, I guess | 00:31 |
| cjwatson | but yeah, seems overkill | 00:31 |
| penguin42 | actually it needs one of the other patches tweeking as well because that is diff'd against it | 00:32 |
| cjwatson | quilt push; quilt refresh your way up the stack | 00:33 |
| cjwatson | but basically trivial | 00:33 |
| penguin42 | I could look at that tomorrow, but it's rather late now, and I can't actually remember any of teh quilt foo | 00:35 |
| cjwatson | if you've tested a patch and confirmed it works, feel free to just attach it to the bug and I can take care of the packaging | 00:37 |
| cjwatson | but you might want the learning experience and if so I don't want to deprive you either :) | 00:37 |
| penguin42 | I'll look tomorrow, or should I say at 1:37 am later today | 00:37 |
| penguin42 | right, bed! | 00:43 |
| === vorian is now known as v | ||
| === v is now known as Obama | ||
| === Obama is now known as vorian | ||
| === vorian is now known as heHATEme | ||
| === Guest89637 is now known as jalcine | ||
| === Guest16852 is now known as vibhav | ||
| YokoZar | https://launchpad.net/ubuntu/precise/+queue :D | 06:52 |
| infinity | YokoZar: Yeah, I'm trying to decide if that was a joke. :P | 06:54 |
| infinity | YokoZar: Do I review it or reject it? | 06:54 |
| YokoZar | infinity: Review it of course, it fulfills its stated purpose ;) | 06:54 |
| infinity | Oh dear... | 06:54 |
| infinity | Oh, derp. April 1. | 06:55 |
| * micahg is curious if a second person signed off on it | 06:58 | |
| jhojho | cjwatson: see that you have been updating openssl. could you add the elliptic curve compile flag for 64bit for the next round? thx. | 07:04 |
| cjwatson | jhojho: no, I haven't got round to talking with the Debian maintainer about it yet | 08:59 |
| Whoopie | cjwatson: Hi, could you perhaps sponsor another package? -> mail-notification, bug 881039 | 09:23 |
| ubottu | Launchpad bug 881039 in mail-notification (Ubuntu) "mail-notification-evolution only contains files in /usr/share/doc" [Undecided,Confirmed] https://launchpad.net/bugs/881039 | 09:23 |
| cjwatson | Whoopie: not right now, need to go out | 09:24 |
| Whoopie | cjwatson: ok | 09:25 |
| Whoopie | have fun | 09:25 |
| cjwatson | I can look later if nobody beats me to it | 09:25 |
| Daviey | Whoopie: you might find that some sponsors are unwilling to upload, without a Firstname Lastname in debian/changelog. | 09:30 |
| Whoopie | Daviey: might be. Even though I see a lot of packages with the same "issue". | 09:31 |
| === tkamppeter_ is now known as tkamppeter | ||
| infinity | Whoopie: It's not terribly common for people to upload with a real name. | 10:06 |
| infinity | Whoopie: But if the code's good, I don't care deeply either. | 10:06 |
| === juliank__ is now known as juliank | ||
| === Pici is now known as ZarroBoogs | ||
| nigelb | ZarroBoogs: awww <3 | 11:18 |
| === jalcine_ is now known as Guest91350 | ||
| === Guest91350 is now known as jalcine | ||
| === dendro-afk is now known as dendrobates | ||
| === dendrobates is now known as dendro-afk | ||
| === bladernr_ is now known as bladernr_afk | ||
| PaoloRotolo | Hi all! | 15:24 |
| === dendro-afk is now known as dendrobates | ||
| === mnepton is now known as mneptok | ||
| === jbicha is now known as Guest20909 | ||
| === Guest20909 is now known as jbicha_ | ||
| hallyn | tjaalton: hey, i thought i'd seen a push of xserver-xorg-video-qxl go by, but I don't see it in the archive? | 16:20 |
| hallyn | (maybe I mis-read a subject and it was another xserver-xorg...) | 16:21 |
| === dantti is now known as dantti_away | ||
| === dendrobates is now known as dendro-afk | ||
| === dendro-afk is now known as dendrobates | ||
| tjaalton | hallyn: you think we can just sync it? I mean it probably needs an FFe bug just to be on the safe side :) | 16:35 |
| hallyn | tjaalton: I don't know, since Munzir hasn't responded I guess I will try installing kubuntu in a guest tomorrow and see if I can (a) reproduce the bug and (b) see it fixed with the new version. | 16:36 |
| hallyn | tjaalton: if it doesn't explicitly fix a bug, then you might be right we should file ffe. | 16:37 |
| hallyn | guess it coudln't hurt to file ffe anyway, and then comment there if it fixes a bug | 16:37 |
| hallyn | (really, spice should be seen as a tech preview right now) | 16:38 |
| tjaalton | hallyn: I don't see any bugs open against -qxl, so this was something reported on irc? | 16:40 |
| hallyn | tjaalton: bug 913314 | 16:40 |
| ubottu | Launchpad bug 913314 in xserver-xorg-video-qxl (Ubuntu) "Corrupted display with Spice" [High,Fix released] https://launchpad.net/bugs/913314 | 16:40 |
| hallyn | after confirmation of the fix of original bug, he lists a few other things still going wrong. | 16:41 |
| tjaalton | ah, ok | 16:41 |
| * penguin42 looks up | 16:44 | |
| === dendrobates is now known as dendro-afk | ||
| === dendro-afk is now known as dendrobates | ||
| === dendrobates is now known as dendro-afk | ||
| === dendro-afk is now known as dendrobates | ||
| === yofel_ is now known as yofel | ||
| JamesJRH | Is it possible for me to obtain a correct source of TAI from my system if I keep it in sync? | 18:05 |
| JamesJRH | Implementing TAI on top of UTC relies on UTC being correctly implemented. But Ubuntu seems to ignore leap seconds in it's implementation of UTC. | 18:06 |
| === dendrobates is now known as dendro-afk | ||
| mamemame187 | hello | 19:19 |
| === jalcine is now known as jalcine_ | ||
| === heHATEme is now known as vorian | ||
| === mwhudson_ is now known as mwhudson | ||
| === nijaba_ is now known as nijaba | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!