| | <<<Back 1 day (to 2020/11/25) | Fwd 1 day (to 2020/11/27)>>> | 20201126 |
| artifexirc-bot | <Robin_Watts> @sebras Yes. That would be much nicer. | 00:26.59 |
| | <Robin_Watts> I had thought of that, but got dragged away from it for some reason... | 00:27.13 |
| | <Robin_Watts> Oh, I think I probably need 'f' as well, which isn't given to me if I use that. | 00:27.41 |
| | <Robin_Watts> I need to talk to @ator tomorrow about whether I should be passing f or field to the js running functions. | 00:28.11 |
| | <ator> @Robin_Watts I believe you need to pass the actual field (not the parent that holds the JS events) | 10:22.36 |
| | <ator> the field argument to pdf_js_event_init function is used as the event.target property, which the JS functions use to get and set various field properties | 10:24.27 |
| | <ator> like setting the textColor in AFNumber_Format based on the negStyle property (whether to parenthesize or color negative numbers) | 10:25.12 |
| | <Robin_Watts> @ator So, if you look at pdf_field_event_format... | 10:42.03 |
| | <Robin_Watts> pdf_execute_js_action is called with 'field' and "AA/F/JS" | 10:42.29 |
| | <Robin_Watts> Which calls pdf_js_execute(doc->js, buf, code) | 10:43.06 |
| | <Robin_Watts> where buf is "%d/%s", where the number of %d comes from field, and %s is the "AA/F/JS". | 10:43.44 |
| | <Robin_Watts> I don't understand the purpose of buf, hence I can't be sure whether it should be number of the actual terminal field object that's used, or whether it should be the object number for the field that contains the "AA/F/JS" entry. | 10:44.46 |
| MP64 | @ator Hi Ator, what JDK do you use to test the java binding and the java viewer sample? The reason I am asking is 64-bit-support. | 10:45.18 |
| artifexirc-bot | <Robin_Watts> code appears to be the code to run, I don't understand what buf is for. | 10:45.33 |
| | <ator> @MP64 openjdk version "11.0.8" 2020-07-14 | 10:46.08 |
| | <Robin_Watts> MP64: The java bindings are tested extensively by our android builds too. | 10:46.18 |
| | <Robin_Watts> And I build/run it using standard Oracle java on windows. | 10:46.32 |
| | <ator> the contents of 'buf' are the "file name" when errors are thrown | 10:48.36 |
| | <Robin_Watts> Ah! | 10:48.50 |
| | <Robin_Watts> OK, so field it is, which means I can use the _inheritable stuff. Fab. Thanks. | 10:49.08 |
| | <sebras> @Robin_Watts thanks for fixing a lot of coverity stuff. | 10:57.39 |
| | <Robin_Watts> @sebras Yeah, the numbers have fallen nicely. | 10:57.59 |
| | <Robin_Watts> 21530{6,7,8} could do with being looked at by someone other than me. | 10:59.08 |
| | <Robin_Watts> 203842 possibly requires a look by @ator though I could patch around it if required. | 10:59.53 |
| | <Robin_Watts> The rest I think I can fix when I get a mo. | 11:00.06 |
| | <sebras> oh! coverity issues, not bug numbers! | 11:00.16 |
| | <Robin_Watts> yeah, sorry 🙂 | 11:00.26 |
| MP64 | @ator Standard Oracle on Windows is 64 bit. So you have a 64-bit javaviewerlib.dll. How do I get one? When I open your Visual Studio file, it is set to 32 bit and compiles fine. When I use the Configuration Manager to set the plattfrom to x64, I get hundreds of compile errors. What do I need to do to get a 64-bit-dll? | 11:01.37 |
| artifexirc-bot | <Robin_Watts> Well, that depends if you have 64 or 32bit windows 🙂 | 11:02.13 |
| | <Robin_Watts> but yes, I think I can build a 64bit javaviewer.dll | 11:02.27 |
| MP64 | I have a 64 bit windows. | 11:02.28 |
| artifexirc-bot | <Robin_Watts> Are you working from git or from a release? | 11:02.36 |
| | <Robin_Watts> IIRC I did a fix last week to make 64bit stuff work. | 11:02.59 |
| MP64 | From git. But it does not matter to me how I obtain a 64 bit javaviewerdll. I simply did not see another way. If it is part of some download package that I missed, I am fine with that. | 11:04.12 |
| artifexirc-bot | <Robin_Watts> From git is good, it means you should have the latest. | 11:04.40 |
| | <Robin_Watts> OK, so in VS2019, from the drop down, I select "DebugJava", and "x64" and rebuild, and lo, I have platform/win32/x64/DebugJava/javaviewerlib64.dll | 11:06.01 |
| sebras | @ator after having read coverity issue 215306 I'm thinking that it complains about us not checking the upper bound of rts in pdf_parse_jbig2_segment_header(). | 11:33.33 |
| | we are only checking if rts == 7, but if we checked rts >= 7 then it might convince coverity? | 11:35.29 |
| | though I can't see how rts can have any value outside of the range [0,7] | 11:36.12 |
| ator | sebras: yeah, I guess it just isn't smart enough to realize the shift reduces the value range | 11:36.41 |
| | so changing it to rts >= 7 should work | 11:36.59 |
| sebras | or should we simply do rts = (data[5] >> 5) & 7 ? | 11:37.02 |
| ator | I hope | 11:37.02 |
| | or that :) | 11:37.15 |
| sebras | I think it complains about us not bounding the tainted data in general. | 11:37.29 |
| | and it can't figure out that we've already done so. or I'm confused by how right shifting unsigned char works and then assigning it to an uint32_t. | 11:38.09 |
| | @ator I think I'm going with the masking, becuase it clearly shows how many bits we expect. | 11:38.55 |
| ator | yes. I think I prefer that too, if it pacifies coverity. | 11:39.31 |
| sebras | I'll try it. | 11:40.34 |
| artifexirc-bot | <Robin_Watts> @ator @sebras So... first we have: https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=9b568b7a11d09d50f32e8dcf0bcd3da7e77a13b6 | 11:54.26 |
| | <Robin_Watts> Then: https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=adafc6709b7a8b84b070cdc7c3a9010ddf47f4a2 | 11:54.37 |
| | <Robin_Watts> bah. Ignore the latter one for a mo. | 11:55.52 |
| | <Robin_Watts> Then: https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=15418bfd9c46ae2a58c62ab06c4940ade3dd1fc2 (Fixed link) | 11:58.27 |
| sebras | @Robin_Watts you accidentally got lcms2 into the second commit. | 12:12.56 |
| artifexirc-bot | <Robin_Watts> @sebras Ass. Thanks. | 12:13.08 |
| sebras | @Robin_Watts I know I am. :) | 12:13.23 |
| artifexirc-bot | <Robin_Watts> https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=15418bfd9c46ae2a58c62ab06c4940ade3dd1fc2 | 12:15.03 |
| sebras | @Robin_Watts its still there? | 12:15.30 |
| | its? it's. | 12:15.39 |
| artifexirc-bot | <Robin_Watts> Ahem: https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=1a72a7af9c4b3b31cacbcd9275f695555749fbe3 | 12:16.01 |
| sebras | @Robin_Watts LGTM "Add pdf_dict_getp_inheritable." | 12:16.09 |
| artifexirc-bot | <Robin_Watts> Pasting the same link twice doesn't work well 🙂 | 12:16.22 |
| sebras | @Robin_Watts the lookups of "AA/K", "AA/V", etc in pdfshow.c should be using the same inheritable call, right? | 12:17.56 |
| artifexirc-bot | <Robin_Watts> yes. | 12:18.08 |
| sebras | maybe in a separate patch if you want to keep this commit separate. | 12:18.18 |
| artifexirc-bot | <Robin_Watts> oh, urm... | 12:18.19 |
| | <Robin_Watts> I'll add that to this patch. | 12:18.28 |
| | <Robin_Watts> Ta. | 12:18.31 |
| | <Robin_Watts> OK. better patch: https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=216a96283d132e9685eb146b35de904fad58d9d0 | 12:20.43 |
| sebras | lgtm. | 12:25.40 |
| artifexirc-bot | <Robin_Watts> Ta. | 12:30.57 |
| | <Robin_Watts> @sebras @ator https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=b361f14719c242784d293843ba570c2e227f4c3b | 12:53.26 |
| | <Robin_Watts> https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=e1f02e7193ce14a087e7c782d222047d05a74d89 | 12:53.37 |
| | <Robin_Watts> https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=a02b387b3c60b71d65c6e85169f1f9eb46359860 | 12:53.47 |
| | <Robin_Watts> @ator So I have the local xref stuff working (for some definition of working). | 13:01.53 |
| | <Robin_Watts> At the moment ALL annotation streams are synthesised 'locally', not into the the document. | 13:02.21 |
| | <Robin_Watts> I'm guessing that there are occasions when we should be going into the document. Any idea when that is? | 13:02.44 |
| | <ator> @Robin_Watts at the top of pdf_update_appearance there's a check if (pdf_obj_is_dirty()) | 13:21.55 |
| | <ator> that sets needs_new_ap which I think is what we can key it off | 13:22.44 |
| | <ator> if needs_new_ap is set, we should put it into the document | 13:22.52 |
| | <ator> if needs_new_ap is not set, and we end up in there, it's because the ap is "missing" | 13:23.22 |
| MP64 | Thank you very much. Have a 64bit Java-lib now the way you described. Thank you also for this lib in general with its great performance and fidelity. | 13:36.51 |
| artifexirc-bot | <paulgardiner> @ator did you get anywhere with bug 703179? I may have misunderstood an earlier comment as implying you were looking at it. | 14:01.01 |
| | <Robin_Watts> MP64: Fab. | 14:04.10 |
| | <Robin_Watts> @paulgardiner If you misunderstood that, so did I 🙂 | 14:04.37 |
| | <Robin_Watts> @paulgardiner He was fiddling in this area yesterday, so I suspect a fix is underway. | 14:06.13 |
| | <Robin_Watts> @ator @paulgardiner So I have annotation synthesis "working" where new annotation appearance streams don't go into the document unless they are caused by a change in value. | 14:35.31 |
| | <Robin_Watts> Which means the undo/redo stuff should all be sane now. | 14:35.45 |
| | <ator> @paulgardiner I'm still fiddling with it | 14:35.47 |
| | <ator> @Robin_Watts nice! | 14:35.58 |
| | <Robin_Watts> @ator commits are on robin/master when you get a chance to look. | 14:36.16 |
| | <Robin_Watts> So this brings me back to the 2 issues that lead me here. "Reload after save" and "safe backgrounding". | 14:38.06 |
| | <Robin_Watts> AIUI the issue with "Reload after save" is that the apps might be holding stuff that is invalidated by saving. | 14:38.31 |
| | <Robin_Watts> AIUI this is to do with pdf_obj's suddenly moving. | 14:38.47 |
| | <Robin_Watts> The apps shouldn't be holding pdf_obj's at all. | 14:38.56 |
| | <Robin_Watts> They should be holding pdf_page's and the like. | 14:39.10 |
| | <Robin_Watts> and the problem is that pdf_pages hold pdf_objs. | 14:39.34 |
| | <Robin_Watts> So... Can we make it so that pdf_pages DON'T hold pdf_objs? But instead hold page numbers? | 14:40.00 |
| | <ator> @Robin_Watts can we not make the process of saving NOT destroy object numbers instead? | 14:44.03 |
| | <ator> move the garbage collection and other destructive options into a separate preprocess pass or something | 14:44.32 |
| | <ator> IIRC it's the renumbering that's the main villain here | 14:45.07 |
| | <ator> everything else we do should be safe | 14:45.45 |
| | <ator> the pdf_page holds pdf_obj that are all indirect references | 14:46.04 |
| | <ator> @paulgardiner I can't reproduce the crash with your file though (but I can reproduce it with another file that crashes in the same location) | 15:02.02 |
| | <Robin_Watts> @ator I'm not sure. | 15:02.07 |
| | <ator> @Robin_Watts changing pdf_page to hold the page number would mean massive slowdowns on big files, because looking through the page tree to find a page object can be very slow | 15:03.09 |
| | <paulgardiner> That's strange. Could it differ between master and 1.18.x? | 15:03.21 |
| | <Robin_Watts> @ator No, I meant have the page hold the object number. | 15:03.39 |
| | <Robin_Watts> That may not help of course. | 15:03.57 |
| | <paulgardiner> ... still, I can test your change when it's done and see if it fixes what I see. | 15:04.16 |
| | <ator> @paulgardiner yeah, for some reason I can't reproduce on master but I can on 1.18.x | 15:04.24 |
| | <ator> @paulgardiner ah, nevermind. it's a double whammy, I fixed *your* file with the other bug fix | 15:05.14 |
| | <ator> and the other file with a second fix | 15:05.23 |
| | <ator> the possible bug that could cause us to desync the scanning with the original document actually causes a desync with your test file, and that triggers the second bug | 15:06.08 |
| | <paulgardiner> Will they cherry pick onto the branch okay, do you imagine? | 15:06.46 |
| | <ator> yeah. either of the two bug fixes I'm working on solves your file. | 15:06.48 |
| | <ator> you'll want both. | 15:06.51 |
| | <ator> both commits are on tor/master now | 15:07.20 |
| | <ator> @Robin_Watts holding a pdf_obj which is an indirect reference number, or the number itself, will both have the same problem | 15:08.24 |
| | <Robin_Watts> Right. | 15:08.34 |
| | <ator> i.e. not solving the issue which is objects being renumbered | 15:08.37 |
| | <ator> one ugly hack we could do is iterate through all "open" pages and renumber their indirect references too. but ick! | 15:09.11 |
| | <Robin_Watts> So when we come to save, and we do a renumber, we shuffle the objects within the xref. | 15:09.25 |
| | <Robin_Watts> How do we alter all the indirect objects that are out there? | 15:09.45 |
| | <ator> we currently alter all indirect objects contained in the document itself | 15:10.37 |
| | <Robin_Watts> i.e. if object 1 is a dictionary that contains an indirect object that points to object 4, and during renumbering, 4 changes to 2. How do we find that indirect object to change it? | 15:10.55 |
| | <ator> we do it in the renumberobj where we iterate through all arrays and dictionaries recursively and create new indirect objects if the numbering has changed | 15:11.14 |
| | <ator> we have the renumber_map to do the renumbering | 15:11.44 |
| | <Robin_Watts> So we do a recursive traversal of everything? right. | 15:11.59 |
| | <ator> yup. | 15:12.04 |
| | <ator> the combination of renumbering and garbage collection could be done by creating a new document and using a graft map and copy the Root object over | 15:12.41 |
| | <Robin_Watts> at the expense of a lot more memory being used. | 15:12.58 |
| | <ator> yeah, we'd make a copy of all the dictionaries and lists | 15:13.17 |
| | <ator> rather than mutate in-place | 15:13.24 |
| | <Robin_Watts> Could we walk the extant pages and change the pdf_obj pointers they are holding too? | 15:13.53 |
| | <ator> maybe if we (a) mutate the pdf_indirect_obj in-place directoly, and (b) loop through open pages and their loaded annotation lists and fixup those indirect referencse with the renumber_map too we could actually get this done | 15:14.12 |
| | <ator> and/or (b) | 15:14.22 |
| | <ator> yes, there's a list of all open pages somewhere in the doc | 15:14.34 |
| | <Robin_Watts> I'm trying to think why just b) isn't enough. | 15:14.51 |
| | <ator> it should | 15:15.10 |
| | <Robin_Watts> We might need to renumber annot->obj's too ? | 15:15.24 |
| | <Robin_Watts> (if they aren't local ones) | 15:15.28 |
| | <ator> but to be safe, mutating in place would also patch up any pdf_obj that have escaped | 15:15.30 |
| | <Robin_Watts> Can you explain what you mean by mutating in place? | 15:15.45 |
| | <ator> yeah, the pdf_page->obj and its annotatinos too | 15:15.50 |
| | <ator> change the number in the indirect reference pdf_obj rather than make a new indirect pdf_obj and replacing it in the dictionary | 15:16.20 |
| | <ator> fz_load_chapter_page has a loop over doc->open which contains all open pages | 15:16.55 |
| | <Robin_Watts> indirect obj's can be referenced from more than once place, right? | 15:16.57 |
| | <ator> yes | 15:17.07 |
| | <Robin_Watts> so we'd have to be careful when rewriting them so as not to rewrite them multiple times. | 15:17.18 |
| | <ator> use the mark/clear thing? | 15:17.42 |
| | <ator> yeah, because the pdf_page ones are indirect references that have been extracted from the page tree (and similarly for the pdf_annot ones) | 15:18.04 |
| | <Robin_Watts> Either we need a 'marked' flag, OR we could move to just having a set of indirect objects that we hold, one per flag. | 15:18.22 |
| | <Robin_Watts> Then we could rewrite them all without having to traverse a tree. | 15:18.43 |
| | <ator> and it would be seriously annoying to have to create a new pdf_obj each time we want to access a page's or annot's obj | 15:18.46 |
| | <ator> what do you mean with "one per flag"? | 15:19.20 |
| | <Robin_Watts> Either we need a 'marked' flag, OR we could move to just having a set of indirect objects that we hold, one per object. | 15:19.33 |
| | <Robin_Watts> I meant one per object, sorry. | 15:19.43 |
| | <Robin_Watts> If we have objects 1 to 100, why don't we also keep a list of indirect objects, one for each of those. | 15:20.04 |
| | <ator> one list of per numbered object? | 15:20.10 |
| | <Robin_Watts> Then whenever we say "create me an indirect object that points to object n", we can just return a reference to one of the preexisting ones. | 15:20.31 |
| | <ator> ah yes, singleton indirect objects | 15:20.47 |
| | <Robin_Watts> Then renumbering is easier - no tree traversal. | 15:21.12 |
| | <ator> would need an extra pointer per xref slot, but I can live with that | 15:21.15 |
| | <ator> means we can't (or should refuse to) create indirect objects that point to objects that don't exist | 15:21.41 |
| | <Robin_Watts> I don't think we can currently do that anyway. | 15:21.54 |
| | <ator> well, when parsing a PDF object we might create them, not sure if we parse before we have the xref | 15:22.18 |
| | <ator> probably only an issue in the repair parsing code | 15:22.26 |
| | <ator> we'll still have to traverse the tree though, or at least make sure to load every object in the file before we start renumbering | 15:24.28 |
| | <ator> to make sure all indirect references in dicts and lists are affected | 15:24.39 |
| | <Robin_Watts> Oh. Right. That takes the bloom off. | 15:25.10 |
| | <Robin_Watts> So, stick a pin in that for a bit. | 15:26.22 |
| | <ator> I suspect we create new objects to replace them in dicts because of this reason, we don't want to renumber twice | 15:27.11 |
| | <Robin_Watts> So, just extend renumbering to pages and annots. | 15:31.48 |
| | <Robin_Watts> I'll look at that. | 15:31.52 |
| | <Robin_Watts> @ator @sebras There are 4 tiny commits on robin/master that would be good to get in today. | 15:38.47 |
| sebras | @Robin_Watts the Coverity ones and mupdf-gl? | 15:39.28 |
| artifexirc-bot | <Robin_Watts> Yup. | 15:40.24 |
| | <Robin_Watts> Woo Hoo! Apparently my HD is fixed. | 15:40.30 |
| sebras | @Robin_Watts congrats! | 15:41.23 |
| | @Robin_Watts in 215180 why did you choose to cast image->n ? is it because it has the smallest type in the expression (uint8_t)? | 15:44.31 |
| artifexirc-bot | <Robin_Watts> a) cos that's what I'd done everywhere else. | 15:44.49 |
| | <Robin_Watts> b) because of my memory being crap. I'll explain. | 15:45.12 |
| sebras | is reading. | 15:45.33 |
| artifexirc-bot | <Robin_Watts> The problem we're solving is that if you do z = a * b, where a and b are ints ,and z is a size_t, C multiplies a*b to get an int, then extends the result to get the size_t. | 15:46.08 |
| sebras | yes, I get that. | 15:46.29 |
| artifexirc-bot | <Robin_Watts> I can never remember if (size_t)a *b is ((size_t)a) * b or (size_t)(a * b) | 15:46.36 |
| | <Robin_Watts> so I do a * (size_t)b cos there is no question in my mind about that. | 15:46.53 |
| sebras | ah! ok, the latter would still be a problem, yet. | 15:47.00 |
| | yes. | 15:47.08 |
| | @Robin_Watts ok, LGTM "Coverity 215180: int * int -> size_t" | 15:47.23 |
| artifexirc-bot | <Robin_Watts> Ta. | 15:47.27 |
| | <Robin_Watts> In the second one, we know sa can never be 0 from earlier in the code. | 15:48.05 |
| | <Robin_Watts> (if sa == 0, then t is read as 256, then we do t = 256-t so t becomes 0, then we check t !=0 to get into this block of code) | 15:48.50 |
| | <Robin_Watts> (the diff is unhelpful in understanding why it's safe) | 15:49.19 |
| sebras | it is, yeah, but now that I know to look at t it is obvious. | 15:50.53 |
| | obvious. | 15:50.55 |
| | @Robin_Watts LGTM "Coverity 116323: Remove logically dead code." | 15:52.01 |
| | LGTM "Coverity 77152: Avoid memcpy of 0 bytes." | 15:52.54 |
| artifexirc-bot | <ator> @Robin_Watts oh yeah, we have disabled buttons now! LGTM Don't have the s/Okay/Open/ button appear/disappear. | 15:53.39 |
| | <Robin_Watts> @ator @sebras Thanks! | 15:53.55 |
| sebras | LGT#metoo! | 15:54.37 |
| artifexirc-bot | <Robin_Watts> 🙂 | 15:55.24 |
| sebras | @ator in pdf_parse_jbig2_segment_header() there is a theoretical upper limit for n, which we don't check. I think this might be the reason for the coverity issue 215307: 4 + 1 + 4 + (0x1FFFFFFF + 1) / 8 + 0x1FFFFFFF * 4 + 4 + 4 | 15:57.02 |
| | even though this is an insanely large value > 2**31 maybe checking for it as an upper bound would quiet coverity. | 15:57.40 |
| artifexirc-bot | <ator> @sebras how do I get to the coverity issue list? I don't have that bookmarked anymore for some reason. | 15:59.52 |
| | <ator> and I find coverity's interface to be horribly confusing | 16:00.51 |
| sebras | @ator I sent you the link privately | 16:01.25 |
| | next click view defects in the upper right | 16:01.33 |
| | the list of issue should be at the top. | 16:01.48 |
| | coverity's interface and error messages could definitely be better. | 16:02.30 |
| artifexirc-bot | <ator> I don't see any defect numbers in that list | 16:02.33 |
| sebras | ok, if you click the cogwheel just above the list you can edit the columns. | 16:03.03 |
| | they are in the second tab in the dialog box. | 16:03.15 |
| artifexirc-bot | <ator> got it | 16:03.29 |
| sebras | @ator I have Action, category, cid, classifiction, component, file, first detected, function, impact, owner, severity, status and type enabled. | 16:04.02 |
| artifexirc-bot | <paulgardiner> @ator would you mind posting the shas for those two commits. I don't want to assume and test the wrong ones. | 16:04.24 |
| sebras | with those I have never lacked anything, but I need to scroll horisontally obviously. | 16:04.30 |
| artifexirc-bot | <ator> @paulgardiner | 16:13.37 |
| | <ator> f720e0c9c (tor/master) Bug 703179: Cope with updating deleted content tags. | 16:13.37 |
| | <ator> 69309a815 Bug 703092: Fix logic error in structured text scanning. | 16:13.39 |
| sebras | @ator I don't think comparing data + n to end would help in pdf_copy_jbig2_segments(). we already do that, but after the fact. | 16:13.42 |
| | so first data += n and then data < end. doing that is apparently not enough. | 16:14.13 |
| | it might be that my fix on sebras/coverity would fix both of these. | 16:14.59 |
| artifexirc-bot | <ator> @sebras hm, so change to if (data + n + info.length > end) | 16:15.52 |
| | <paulgardiner> Ha! I hadn't realised they were right at the end of your branch 🙂 | 16:16.05 |
| sebras | @ator well, I can _add_ such a check, I can't change the while()-expression I think. | 16:16.32 |
| artifexirc-bot | <ator> there is a check like that which throws "truncated jbig2 segment data" error | 16:16.58 |
| | <ator> but see if your &7 check works first | 16:17.04 |
| | <ator> because I'm fairly confident myself that pdf_parse_jbig2_segment_header checks the same range | 16:17.51 |
| | <ator> so that we know that 'n' is good, and there is enough header data to copy | 16:18.13 |
| sebras | @ator ok, I assume that's LGTM on sebras/coverity? | 16:18.20 |
| artifexirc-bot | <ator> y. LGTM to Coverity 215306: Limit field size. | 16:18.49 |
| sebras | @ator well. if you get n == 2214592525 back from pdf_parse_jbig2_segment_header() we _do_ add this to data before we're checking whether this would overflow end..? | 16:19.12 |
| artifexirc-bot | <ator> if (data + n + 4 > end) return 0; in pdf_parse_jbig2_segment_header | 16:20.04 |
| sebras | @ator if data ends up at a high memory address and n is large, could we end up with data + n + 4 overflowing? | 16:22.04 |
| | would checks like these be better as n + 1 <= end - data or something like that? | 16:22.59 |
| artifexirc-bot | <ator> isn't that true for pretty much every single use of pointer arithmetic then? | 16:23.25 |
| sebras | it would be, yes. | 16:23.34 |
| artifexirc-bot | <ator> I give up. Wanna do rm -rf ~/src/mupdf and rewrite everything in a sane language. | 16:23.43 |
| sebras | rewrite it in javascript and run it in mujs! :) | 16:24.02 |
| artifexirc-bot | <ator> I said sane language, silly! :P | 16:24.15 |
| sebras | @ator hey, _you_ wrote mujs, the language it interprets must be sane by inference. :) | 16:24.50 |
| | @ator tbh I'm not sure whether rewriting mupdf in rust/go/whatever-is-cool-now wouldn't come with its own set of problems. | 16:25.35 |
| artifexirc-bot | <ator> as long as whatever-is-cool-now doesn't have deathtraps of undefined behavior and overflow until you puke issues everywhere it can't possibly be any worse than C | 16:26.36 |
| | <paulgardiner> @ator yep, that sorts it, thanks. Any objection to my pushing those commits to the end of 1.18.x? | 16:28.02 |
| | <ator> @paulgardiner no objections | 16:29.13 |
| sebras | maybe it will unexpectedly throw unchecked exceptions. | 16:29.52 |
| ator | better that than a deluge of CVE security researchers descending like hungry flies on a fresh dog turd... | 16:30.49 |
| sebras | @ator I see that pointer arithmetic was a sensitive subject, sorry to bring it up. :) | 16:32.36 |
| | I blame coverity. | 16:32.57 |
| ator | no. let's just change ALL variables to int64_t and add a if (x > INT_MAX || x < INT_MIN) fz_throw("YOU STUPID STUPID FOOL WHY DID YOU LET IT OVERFLOW!") to every line of arithmetic. sound good? | 16:33.58 |
| | it just feels like we've wasted sooo much time on stupid overflow issues these last years. | 16:35.54 |
| sebras | not really, no. I agree with your frustration, I feel it myself. | 16:36.18 |
| | we have, as long as we run coverity/oss-fuzz things like this are bound to creep up. and as long as there are CVE researchers they will report these, even if we don't run the tools ourselves. | 16:37.14 |
| | and this all detracts from implementing things we need to/want to add to mupdf. | 16:37.45 |
| | I like how 215308 assumes that the first segment is type 51 (end of file) and then assumes type 49 (end of page). that logic is not coherent, but coverity can't see that. | 16:40.22 |
| | regardless 215308 and 215307 are quite similar, so I think I'll wait for coverity to rerun and see what it says. | 16:40.49 |
| | I'll take a look at the 10 open oss-fuzz issues next. | 16:46.59 |
| | @ator that took some time to understand, but now it LGTM "Bug 703092: Fix logic error in structured text scanning." | 23:03.14 |
| | I'll wait with 703179 until tomorrow. | 23:04.32 |
| | <<<Back 1 day (to 2020/11/25) | Forward 1 day (to 2020/11/27)>>> | |
Log of #mupdf at irc.freenode.net.