| | <<<Back 1 day (to 2020/11/26) | Fwd 1 day (to 2020/11/28)>>> | 20201127 |
| Kabouik | Hey there. Is there a way to quickly show an exploded view in mupdf, with all pages displayed at once as thumbnails? Not something I critically need, but curious. | 09:37.31 |
| | I don't think so but it's worth asking. | 09:38.17 |
| ator | Kabouik: afraid not, no. | 09:38.53 |
| Kabouik | No problem! Not the most important for me, I miss more the ability to save pdfs I open as temporary files for example, and want to save somewhere else (it's there in the F1 help and there's a dialog, but I get an error every time I try; also it doesn't remember the path I tried when it fails, so it's a bit cumbersome to try multiple times) | 09:42.31 |
| artifexirc-bot | <Robin_Watts> @sebras @ator We're down to 5 coverity issues now. 4 of them are tainted stuff for which my attempted fixes have not worked. Any ideas gratefully accepted. | 09:49.39 |
| sebras | @Robin_Watts I tried fixing 215306 yesterday. | 09:51.40 |
| artifexirc-bot | <Robin_Watts> You did, and that worked. | 10:01.52 |
| sebras | @Robin_Watts nice! it didn't fix 215307 and 215308 though. I was hoping that it would :-/ | 10:13.48 |
| artifexirc-bot | <Robin_Watts> Yeah 😦 | 10:14.06 |
| sebras | oh, but now it complains not about "data" being tainted, but info.lenght! | 10:14.46 |
| | yet they keep the same issue number, why?! | 10:15.02 |
| | @Robin_Watts btw, I've tried reproducing the 10 open oss-fuzz issue and I ran into problems in all but two. | 10:17.05 |
| artifexirc-bot | <Robin_Watts> "ran into problems" = could reproduce ? | 10:17.33 |
| sebras | no, could _not_ reproduce. | 10:17.47 |
| | the two I can reproduce were both related to jbig2dec. I'm working on one of them. | 10:18.10 |
| artifexirc-bot | <Robin_Watts> right. | 10:18.49 |
| sebras | @Robin_Watts can I get a nod on sebras/coverity? | 10:25.44 |
| artifexirc-bot | <Robin_Watts> 2 mins. | 10:26.09 |
| sebras | an option is to do the masking in getu32(), maybe that is better? | 10:30.04 |
| | the data[n] & 0xFF should probably remain though. | 10:30.23 |
| artifexirc-bot | <Robin_Watts> sebras: getu32 has a coverity marker before it. | 10:31.24 |
| | <Robin_Watts> I added that in the hopes that this would make this stuff behave. | 10:31.37 |
| | <Robin_Watts> We should remove it. | 10:31.45 |
| | <Robin_Watts> We should remove it (given it doesn't appear to actually work) | 10:31.59 |
| | <Robin_Watts> Like @ator I like the idea of adding the masking there (and leaving the & 0xFF where it is). | 10:32.27 |
| sebras | ok, I'll fix that. | 10:32.49 |
| artifexirc-bot | <Robin_Watts> We could even #ifdef COVERITY the masking, to make it clear that it's really just something we're doing for coverity? | 10:33.02 |
| sebras | I'd find that more distracting than a slightly redundant masking. | 10:34.05 |
| artifexirc-bot | <Robin_Watts> yeah, but if we're doing it just in 1 place, it's OK, I reckon. | 10:36.31 |
| | <Robin_Watts> (In one of the other places, possibly, gif or bmp, I tried for a safe_getu32 which was supposed to mark to coverity that the value was safe, but I don't think that worked). | 10:37.14 |
| | <Robin_Watts> Whatever you reckon, anyway. | 10:37.20 |
| sebras | @Robin_Watts The masking seems to have worked yesterday, so I'll go for that. | 10:37.54 |
| | @Robin_Watts gcc on amd64 compiles getu32() to the same code whether it has the masking or not. the debug build adds an instruction of course. | 10:44.18 |
| | sebras/coverity updated. | 10:45.21 |
| | I'll have a look at the bmp/gif ones too. | 10:45.37 |
| artifexirc-bot | <Robin_Watts> sebras: lgtm. | 10:52.47 |
| ator | sebras: wtf is going on with "Coverity 215307/215308: Explicitly limit ranges of read fields by masking."? | 11:18.44 |
| | those masks are 100% no-ops so why could they possibly help with anything? | 11:18.57 |
| sebras | the fuck (yes, ray you may edit the logs) that is going on is that Coverity reasons that data comes from an external source because getu32() does byte swapping. this then tains info->length in pdf_parse_jbig2_segment_header(). getu32() returns an uint32_t, and info->length is an int. I'm assuming that Coverity is worried that we do not validate the range of info->length. this tainting then propagates to the | 11:27.53 |
| | calling function pdf_copy_jbig2_segments(), resulting in 215307 when info.length is passed to fz_append_data(). if you have a better idea I'm all eyes. | 11:27.59 |
| | I don't know how Coverity works or exactly what triggers its tainted issues. even the masking I added to rts yesterday is redundant. we both know that. | 11:30.04 |
| | but apparently coverity does not. | 11:30.25 |
| | a potential way of fixing these is classifying them as False Positive. I'm not sure whether that will stick when/if the code is changed slightly. | 11:31.34 |
| | also, don't worry I haven't pushed anything. | 11:32.08 |
| ator | that's just so insanely dumb it's hard to believe coverity would treat masking with the full range a valid untainter.... | 11:33.39 |
| | sebras: is coverity happy with |0 too then? | 11:34.09 |
| sebras | coverity _did_ close 215306 after the &0x7 that you okayed yesterday. | 11:34.33 |
| ator | the &0x7 I can understand. it's forcing a range that was checked using logic that isn't obvious to a computer algorithm, so while redundant it's also not completely misguided :) | 11:35.40 |
| sebras | I think what it is looking for is something that limits the range of the external data. I guess less/greater than comparisons would work, and the commit yesterday proved that masking is recognized as well. | 11:36.00 |
| ator | well, that masking is not limiting the range of the data, so again, wtf? | 11:36.38 |
| | could there be a deeper coverity issue here that the masking just hides (pardon the pun)? | 11:37.21 |
| artifexirc-bot | <Robin_Watts> False positives in Coverity are a bad thing, generally. | 11:39.15 |
| sebras | how does taking an unsigned char, shifting it five bits to the right, not leave a three bit value? what does &0x7 do that had not already happened? if it was signed value getting shifted I'd understand it, but it is not. | 11:39.30 |
| | ator: had there been any reasonable documentation about coverity and this particular issue I'd be happy to read it. I haven't found any. | 11:41.06 |
| artifexirc-bot | <Robin_Watts> sebras: If the masking works, then possibly we can do something like: | 11:47.45 |
| sebras | I don't know if will work in this case. | 11:48.10 |
| | and if I want to test I will have to push the commit and wait for coverity to rerun. it might be possible for me to start a separate private project where I can upload my own private runs. | 11:49.11 |
| | that way I won't have to taint origin/master with my attempts. | 11:49.36 |
| artifexirc-bot | <Robin_Watts> #ifdef COVERITY #define DETAINT(V,BITS) ((BITS == 32) ? (V & 0xFFFFFFFFU) : (V & ((1U<<((BITS)+1)-1))) #else #define DETAINT(V,BITS) (V) #endif | 11:49.43 |
| ator | sebras: you're right, same stupidity with the &0x7 :) | 11:50.26 |
| artifexirc-bot | <Robin_Watts> That could live in a header, and we could then just "DETAINT(V,8)" in the code, so it'd be limited interference at each site. | 11:50.51 |
| ator | could some of this be down to aliasing, it can't trust that a variable won't be changed if it comes from a struct? | 11:50.52 |
| sebras | ator: I know. I'm not smarter than this. | 11:50.59 |
| artifexirc-bot | <Robin_Watts> ator: I think you're looking beyond the "Coverity is frequently crap" explanation. | 11:51.50 |
| | <Robin_Watts> which is all that is required here. | 11:51.58 |
| sebras | ator: any variable with which you do byte swaping will be considered to come from an external source. all discussions about TAINTED_SCALAR I've seen so far indicates that one has to check the range of the value. I'm not sure how to tell coverity I know this is the entire range, and I want it all. | 11:52.56 |
| artifexirc-bot | <Robin_Watts> sebras: This is all the documentation I could find: https://scan9.coverity.com/doc/en/cov_checker_ref.html | 11:55.53 |
| | <Robin_Watts> It was based on that document that I tried the: // coverity[ -tainted_data_return ] | 12:00.20 |
| | <Robin_Watts> Is it possible that that marking didn't work, just because of whitespace differences? 😦 | 12:01.20 |
| | <Robin_Watts> Possibly we should post to a coverity message board and ask? | 12:02.10 |
| sebras | there TAINTED_SCALAR/Untrusted value as argument tells me that "The argument could be controlelr by an attacker, who could invoke the function with arbitrary values (e.g. a very high or negative bufer size)." | 12:02.44 |
| artifexirc-bot | <Robin_Watts> Yes, that's the whole argument about tainted data. | 12:05.08 |
| | <Robin_Watts> If you're reading 'external' data, then you can't trust it. And that principle is correct. It's just that coverity's mechanism for detecting/reasoning about it are flawed. | 12:05.49 |
| | <Robin_Watts> I just saw a gs bug where we do: void *result = NULL; void **presult = &result; .... *presult = something; .... if (result) ... and coverity was complaining that result must always be NULL at that point. | 12:07.08 |
| | <Robin_Watts> so there are fairly basic problems with its reasoning in some cases. | 12:07.39 |
| sebras | it it is guessing that result can be NULL becuase of the if. | 12:10.18 |
| artifexirc-bot | <Robin_Watts> oops. if was if (!result) at the end, and it was complaining that the code inside the if can never be reached. | 12:16.53 |
| | <Robin_Watts> I can cope with static analysis tools that say "this looks suspicious", but tools that say "this can never happen" when they clearly can, are bad. | 12:17.55 |
| | <Robin_Watts> if (0) { label: do_something(); } ..... if (some_condition) goto label; ... is another thing that coverity gets wrong. | 12:18.36 |
| | <Robin_Watts> It claims that the inside of the first if is dead code. | 12:18.49 |
| | <Robin_Watts> @ator bugger. I hit a problem with the local_xref stuff. | 16:25.03 |
| | <Robin_Watts> If I'm generating a new appearance stream, I optionally push a local_xref. | 16:25.30 |
| | <Robin_Watts> That means all new objects go into that xref. | 16:25.40 |
| | <Robin_Watts> So we regenerate the appearance stream, and that needs a font. | 16:25.53 |
| | <Robin_Watts> So we create the font objects etc in the local_xref as you'd expect. | 16:26.04 |
| | <Robin_Watts> but we then write that back into the resource dictionary, which is not local. | 16:26.34 |
| | <ator> the resource dictionary for the appearance stream should be local too | 16:32.21 |
| | <Robin_Watts> Yeah. | 16:32.25 |
| | <Robin_Watts> How? | 16:32.38 |
| | <ator> in pdf_update_appearance we do everything to create the appearance stream. it calls pdf_write_appearance to create the buffer and resource dictionary, and then it fiddles with the Rect and Matrix stuff and calls pdf_new_xobject or pdf_update_xobject to create the final appearance stream pdf object | 16:35.20 |
| | <ator> where have you inserted the push/pop local_xref? | 16:35.43 |
| | <Robin_Watts> @ator push/pop is in pdf_update_appearance | 16:47.37 |
| | <Robin_Watts> Somehow stuff being created for one annotation is being seen by the next one. | 16:53.53 |
| | <ator> does the store and/or resource cache take the local_xref into account? | 16:55.21 |
| | <Robin_Watts> Not at the moment. | 16:55.46 |
| | <ator> then I would expect font resources created for one annotation to be reused for the next | 16:56.07 |
| | <Robin_Watts> Ok, I can't quite see how, currently... | 16:56.20 |
| | <ator> write_variable_text calls add_required_fonts calls pdf_add_simple_font which calls pdf_find_font_resource | 16:57.39 |
| | <Robin_Watts> pdf_find_font_resource | 16:57.55 |
| | <ator> and pdf_insert_font_resource | 16:58.01 |
| | <Robin_Watts> Yup. I see it. Thanks. | 16:58.05 |
| | <Robin_Watts> Fab. | 16:58.17 |
| | <Robin_Watts> @ator OK, so new stuff on robin/master. That might get us saving without needing to reload. | 17:10.31 |
| | <Robin_Watts> We're leaking a lot. | 17:12.00 |
| | <Robin_Watts> Not entirely happy with just EORing into the digest for the font resource stuff. I may tweak that a bit. | 17:12.21 |
| | <Robin_Watts> @ator So, if I load TestFormExpenses.pdf into mupdf-gl, and then quit it, we leak some pdf_field_name's. | 17:33.07 |
| | <Robin_Watts> I think that's a JS thing that's causing them to leak. | 17:33.54 |
| | <cgdae> ok, i think i have a working gs docxwrite device: https://git.ghostscript.com/?p=user/julian/ghostpdl.git;a=summary | 18:09.51 |
| | <Robin_Watts> nice. wrong group, but nice 🙂 | 18:19.42 |
| | <cgdae> oh! | 18:20.02 |
| | <Robin_Watts> OK, my leaks are fixed. We still have the field name leak from js, but I'll talk to @ator about that on monday. | 19:00.28 |
| | <<<Back 1 day (to 2020/11/26) | Forward 1 day (to 2020/11/28)>>> | |
Log of #mupdf at irc.freenode.net.