| | <<<Back 1 day (to 2020/01/12) | Fwd 1 day (to 2020/01/14)>>> | 20200113 |
| Robin_Watts | ator, paulgardiner: More fun and excitement with validation stuff: https://git.ghostscript.com/?p=user/robin/mupdf.git;a=shortlog;h=refs/heads/validate-form-changes | 15:14.41 |
| | The first 4 commits I think have been looked at before. | 15:15.00 |
| | But I'd like to get them in now (so if someone could nod at them again, that'd be great). | 15:15.56 |
| | The next one is a useful utility function that you may have opinions on. | 15:16.15 |
| | The next one is the updated validation one that seems to work on the examples I've tried. | 15:16.42 |
| | and I'm going to keep tweaking the GL one for a bit. | 15:16.56 |
| ator | might want to have sebras look through the size_t one, he's a lot better at spotting issues with that sort of thing than I am | 15:18.25 |
| | we're always going to be at odds with typedef enum. I like it being blindingly obvious that it's an enum and not a struct. | 15:19.53 |
| Robin_Watts | It compiles without warnings on VS and Linux. | 15:20.03 |
| | We use typedef enum *EVERYWHERE* in mupdf. | 15:20.19 |
| ator | I know, so I'm going to let it pass (but as you noticed, I'll still grumble) | 15:20.54 |
| Robin_Watts | ok :) | 15:21.09 |
| | grumble accepted. | 15:21.14 |
| ator | pdf_walk_tree_kid calls the callback for non-leaf nodes too | 15:22.26 |
| Robin_Watts | ator: It does. | 15:22.39 |
| | (deliberately, I think) | 15:25.20 |
| ator | name trees forbid Names on nodes that have Kids | 15:26.08 |
| | so all content must be in leaf nodes (or the root node if it's also a leaf node) | 15:26.23 |
| Robin_Watts | ator: This is to be used for trees of annots. | 15:26.36 |
| ator | same with number trees | 15:26.38 |
| Robin_Watts | which can have /V at any point. | 15:26.46 |
| ator | The AcroForm /Fields array? | 15:26.58 |
| Robin_Watts | could be. | 15:27.06 |
| ator | okay, so basically any tree-like structure that has a "Kids" entry that may or may not be an array? | 15:27.34 |
| Robin_Watts | so if you're wanting to walk a tree and process /V's as you go, getting called for non-leaf is important. | 15:27.35 |
| | Yes. | 15:27.39 |
| ator | right. do we care about the order (do we call fn before or after walking the children?) | 15:27.54 |
| Robin_Watts | We *could* have 2 callbacks, one for leaf, one for non-leaf ? | 15:28.05 |
| ator | I'd assume we want to call it before walking the kids if processing fields | 15:28.06 |
| | nah, should be fine I think for PDF | 15:28.19 |
| | if it matters, you can distinguish leaf and non-leaf by the presence or absence of what you're looking at in the callback | 15:28.39 |
| Robin_Watts | We call the callback after the kids currently. I can change that. | 15:28.55 |
| ator | I think if say pritning or flattening the field hierarchy, having parents before kids makes sense | 15:29.21 |
| Robin_Watts | ok. | 15:29.26 |
| ator | but I don't have a strong opinion, if you have a use case that works the way it is now I'm fine with that too | 15:29.49 |
| sebras | Robin_Watts: for "Squash warning in x64 build of mu-office-test." are we asserting on cookie to make sure that we don't get a warning about cookie being unused? using intptr_t all over seem reasonable though. also why are load_progress() and load_error() both 1234 whilc render_progress() is 5678, wouldn't having separate value for each call be even better? | 15:29.55 |
| Robin_Watts | ator: updated. | 15:30.27 |
| | sebras: looking | 15:30.30 |
| | sebras: It's intended more as a demonstration to people reading the code that the value passes through (cos it's kinda example code) | 15:31.24 |
| sebras | Robin_Watts: Re: "Fix 64bit MSVS builds of OpenSSL variants." I can't have too much useful input on. I simply trust you did it right. :) | 15:31.27 |
| Robin_Watts | I can't comment on why I picked those values. Cut and Paste I suspect. | 15:31.52 |
| sebras | Robin_Watts: right, then they could all have been the same value or unique, it doesn't really matter. the commit itself LGTM. | 15:32.24 |
| | Robin_Watts: the msvs build also LGTM. | 15:32.37 |
| Robin_Watts | Ta. | 15:32.42 |
| sebras | ator: git checkout robin/master; make tags; vim -t pdf_supports_signatures; E431: Format error in tags file "tags" Before byte 54 E426: tag not found: pdf_supports_signatures | 15:45.15 |
| | and then head -15 tags lists !_TAG_FILE_FORMAT only on the 13th line. | 15:45.46 |
| | ator: is this something you've experienced? | 15:45.52 |
| | it happens repeatedly here, but I don't know why. | 15:46.17 |
| | and rm tags; ctags -R causes the same issue. | 15:46.47 |
| | and no LC_COLLATE LANG and LANGUAGE are not involved (unsetting them results in the same issue). | 15:49.06 |
| ator | I cannot find pdf_supports_signatures with grep | 15:50.31 |
| | I think you mean checkout robin/validate-form-changes | 15:51.12 |
| | in which case, no, I can run vim -t pdf_supports_signatures with the expected results | 15:51.33 |
| sebras | ator: yes, that's what I meant, and that's what I did, just not what I wrote. | 15:51.34 |
| Robin_Watts | pdf_supports_signatures is in libpkcs7 thing. | 15:52.25 |
| | source/helpers/pkcs7/pkcs7-check.c | 15:52.48 |
| ator | sebras: Exuberant Ctags 5.9~svn20110310 | 15:53.03 |
| sebras | ator: I found the offender. I have a stringprep directory in ~/src/mupdf and its contents causes ctags to misbehave. | 15:53.39 |
| | I have the same ctags btw. | 15:53.51 |
| | ator: found out why. I have been looking at possibly implementing stringprep before, when doing so I was looking at heimdal that implements stringprep. when closning that I also got this file: https://github.com/heimdal/heimdal/blob/master/lib/hcrypto/libtommath/tommath.tex | 16:07.15 |
| | ator: and ctags tries to process that file. :( | 16:07.29 |
| ator | Robin_Watts: several warnings with "e52497273 Add functions to validate changes within a PDF file." on linux with clang | 16:10.43 |
| | source/pdf/pdf-object.c:384:13: warning: comparison of unsigned expression < 0 is always false | 16:11.02 |
| Robin_Watts | Ta, will look. | 16:11.03 |
| ator | source/helpers/pkcs7/pkcs7-openssl.c:257:33: error: incomplete result type 'enum pdf_signature_error' in function definition | 16:11.09 |
| | source/helpers/pkcs7/pkcs7-openssl.c:318:9: error: returning 'int' from a function with incompatible result type 'enum pdf_signature_error' | 16:11.15 |
| | source/helpers/pkcs7/pkcs7-openssl.c:418:19: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') | 16:11.22 |
| | source/helpers/pkcs7/pkcs7-openssl.c:450:19: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') | 16:11.29 |
| | source/helpers/pkcs7/pkcs7-openssl.c:818:19: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') | 16:11.34 |
| | actually, that last one is on origin/master too now | 16:12.19 |
| | seems like the #ifndef HAVE_LIBCRYPTO else- | 16:12.54 |
| | branch has not been updated | 16:12.59 |
| | we should back out 713f1d693 from origin/master IMO. I'm surprised it even clustered. | 16:13.55 |
| | do we not have openssl installed on the cluster machines? | 16:14.09 |
| Robin_Watts | on phone, sorry | 16:17.25 |
| sebras | ator: I agree, how did 713f1d693 end up on master? wasn't that the one I was supposed to look at? | 16:21.29 |
| | (I was still looking, getting side tracked by ctags misbehaving) | 16:21.49 |
| | ator: gcc 9.2.1-21 complains the same way. | 16:22.22 |
| | Robin_Watts: can't we use SIZE_MAX for the length check in pkcs7_openssl_check_digest()? we already use SIZE_MAX in source/fitz/store.c::fz_store_scavenge(). | 16:25.26 |
| Robin_Watts | f7401522b was the one for sebras to look at (still on phone) | 16:26.56 |
| sebras | Robin_Watts: hm. or even better INT_MAX since that's what we cast it to. | 16:27.02 |
| | ator: will you back 713f1d693 out in the mean time? | 16:27.55 |
| | ator: or do we wait for Robin? | 16:28.00 |
| ator | I'm going to be lazy and let Robin do it, so I don't have to look up how to fix the cluster afterwards :) | 16:28.40 |
| sebras | ator: ok. | 16:29.50 |
| | ator: btw, I wanted signed bignums everywhere. | 16:30.59 |
| ator | sebras: I want to give up C altogether, and its insane integer promotion madness | 16:31.31 |
| sebras | ator: maybe we should be looking into reimplementing all of this is lisp? | 16:31.49 |
| | ator: I'm happy we have tools helping us, but it's annoying that we can't get rid of this whole class of bugs and then prevent committing the same bugs again in the future. | 16:32.45 |
| ator | Go or Rust seems to be two ways of doing it, with plenty of drawbacks of their own. | 16:33.46 |
| | like rewriting the whole code base. | 16:34.03 |
| sebras | ator: I know, I'm not really being serious. | 16:34.56 |
| Robin_Watts | ok, back. | 16:36.37 |
| | so let me see what I did wrong in that commit. | 16:36.44 |
| | Oh, i see, I screwed it up cos I rebased the order of commits. | 16:37.17 |
| sebras | git grep -B2 'too large' source/ reveals that we usually use _MAX for comparisons in these cases. using INT_MAX here seems sane to me. | 16:37.19 |
| Robin_Watts | will fix. | 16:37.27 |
| sebras | Robin_Watts: ok. let me know when it's done. | 16:37.35 |
| | Robin_Watts: will you use INT_MAX too? | 16:37.43 |
| Robin_Watts | I have yet to understand that part. | 16:37.59 |
| | ok, force pushed. | 16:40.59 |
| | cluster reset | 16:41.05 |
| sebras | Robin_Watts: we cast sig_len to int before passing to BIO_new_mem_buf(), so we ought to check sig_len against INT_MAX. | 16:41.25 |
| Robin_Watts | And if it's not OK, what do we do? | 16:50.39 |
| sebras | Robin_Watts: why wouldn't that be ok? | 16:51.24 |
| | Robin_Watts: or what do you mean? you add a fz_throw() now, and that's ok by me. | 16:51.37 |
| Robin_Watts | right. fz_throw. | 16:51.45 |
| sebras | Robin_Watts: hm.. wait. can we do that? | 16:52.20 |
| | perhaps this ties in with pedros changes which also use these things. | 16:52.52 |
| Robin_Watts | OK, updated tree: https://git.ghostscript.com/?p=user/robin/mupdf.git;a=shortlog;h=refs/heads/validate-form-changes | 16:54.09 |
| | https://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=4aba68cf41f5430a8b98c94f71e70c38615ae263 that is now much simpler. | 16:54.26 |
| | The size_t changes are deferred to the next commit with all the others. | 16:54.44 |
| sebras | looks. | 16:56.05 |
| | Robin_Watts: I worry that we typedef here only because I know pedro is already looking at my halfdone API changes. | 16:57.00 |
| | Robin_Watts: it would be trivial to fix though. | 16:57.24 |
| Robin_Watts | yeah. That'll be the least of his problems :) | 16:57.53 |
| | It's the right thing to do longterm, so he'd have to take it on at some point (IMAO) | 16:58.22 |
| sebras | Robin_Watts: "Move pdf_signature_error to be a typedef enum." is now indeed much simpler and I think we can push that. | 16:58.31 |
| Robin_Watts | Ta. | 16:58.37 |
| | Just waiting for a confirmatory cluster test and I'll push. | 16:58.53 |
| | Now to look at your INT_MAX issue. | 16:59.03 |
| sebras | Robin_Watts: re typedef vs enum: I can probably be convinced either way. I don't really know what the pros/cons are here. | 16:59.30 |
| Robin_Watts | sebras: Less to type :) | 16:59.53 |
| sebras | thanks for merging all the size_t changes into a single commit. that makes more sense. :) | 17:00.02 |
| Robin_Watts | yeah, I should have done it like that to start with, but I reordered them to get the "less contentious" ones in first. | 17:00.39 |
| sebras | Robin_Watts: ator's point about clarity is not wrong either. though I personally use ctags (as seen before) so I mostly have to look up non standard types anyway. | 17:01.22 |
| | and when it is a return value without a pointer it is quite unlikely to be struct. | 17:01.35 |
| Robin_Watts | sebras: There is an argument that says it shouldn't matter if it's a struct or an enum. It's conceptually a "thing". It only matters when you compare it to something, at which point it's obvious. | 17:02.40 |
| | In the same way that I hate saying "struct foo" everywhere, I hate saying "enum foo". | 17:03.16 |
| | But yes, this may be a matter of personal taste. | 17:03.25 |
| sebras | Robin_Watts: I think it might be, yes. | 17:03.35 |
| | personally I don't mind typing out struct/enum so for me that is non-issue. | 17:04.04 |
| Robin_Watts | All those stray "enum"s read like warts in the source code :) | 17:04.12 |
| sebras | anyway, your argument about us using typedef for enums elsewhere makes sense to me. | 17:04.35 |
| | Robin_Watts: Re: warts :) | 17:05.02 |
| | Robin_Watts: did you sort out the INT_MAX issue? | 17:05.20 |
| | wait.. | 17:05.55 |
| | you have pdf_check_digest() returning enum pdf_signaure_error in 4aba68cf4 | 17:06.55 |
| | this causes a compilation error now. | 17:07.03 |
| Robin_Watts | yeah, cluster test just failed for that. will fix. | 17:16.08 |
| | OK, new version up, reclustering... | 17:21.22 |
| sebras | Robin_Watts: 4e2132762 has issues to. you forgot to update include/mupdf/helpers/pkcs7-check.h with size_t | 17:37.11 |
| | and include/mupdf/helpers/pkcs7-openssl.h | 17:37.32 |
| Robin_Watts | yeah, I'm working on that now. git merges swallowed some changes :( | 17:38.09 |
| sebras | it seems like you forgot int->size_t in pkcs7_openssl_check_digest() | 17:38.13 |
| | rebase or merge? | 17:38.36 |
| sebras | never merges, hence the surprise. | 17:38.45 |
| Robin_Watts | rebase | 17:39.18 |
| sebras | fixing those things leaves thre compilation warnings in pkcs7_openssl_check_certificate(), pkcs7_openssl_check_digest() and pkcs7_openssl_designated_name(). | 17:39.28 |
| | Robin_Watts: ok. | 17:39.31 |
| Robin_Watts | when it resolves conflicts, it swallowed stuff. | 17:39.36 |
| sebras | I'll pop out for second. | 17:39.38 |
| | I'll be back in a bit. | 17:39.52 |
| | back. | 18:05.59 |
| | Robin_Watts: how about if (sig_len > (size_t) INT_MAX) instead of if ((int)sig_len != sig_len) ? | 18:07.25 |
| Robin_Watts | You prefer that? | 18:07.35 |
| sebras | Robin_Watts: doesn't it better express what we want to do? | 18:07.49 |
| Robin_Watts | Any function reason, or just stylistic? | 18:07.54 |
| | does it? | 18:07.57 |
| sebras | I get warnings | 18:08.02 |
| Robin_Watts | If ((int)sig_len != sig_len) expresses exactly what I want to check. | 18:08.18 |
| sebras | source/helpers/pkcs7/pkcs7-openssl.c:450:19: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare] | 18:08.20 |
| Robin_Watts | If "sig_len as an int" is different to "sig_len as a size_t"... | 18:08.48 |
| | well, that's a good reason :) | 18:08.55 |
| | I'll update to your preferred formulation. | 18:09.08 |
| | Just fighting stuff on peeved so I can do test builds there. | 18:09.22 |
| sebras | I'm not sure mine is better, but something needs to change for the warning to go away. | 18:09.25 |
| | Robin_Watts: sure, no problems. | 18:09.36 |
| | Robin_Watts: tyt | 18:09.42 |
| Robin_Watts | pkg-config on peeved appears to find none of the packages we use. | 18:18.50 |
| | not freetype, certainly not libcrypto | 18:19.15 |
| sebras | Robin_Watts: isn't freetype cloned recursively? | 18:19.37 |
| Robin_Watts | Yes, but libcrypto isn't. | 18:20.12 |
| | but sudo apt-get install libcrypto tells me it's already installed. | 18:20.39 |
| sebras | Robin_Watts: Robin_Watts: aren't we testing for that with pkg-config --exists 'libcrypto >= 1.1.0' and then not setting HAVE_LIBCRYPTO if it is the wrong version? | 18:20.51 |
| Robin_Watts | but pkg-config --exists libcrypto doesn't fins it. | 18:20.57 |
| | so I tried to pkg-config --exists freetype, and even that isn't found. | 18:21.15 |
| sebras | that's freetype2, isnt it? | 18:21.41 |
| | Robin_Watts: you installed libssl-dev right? | 18:22.14 |
| | to get libcrypto.pc in /usr/lib/x86_64-linux-gnu/pkgconfig/libcrypto.pc ? | 18:22.30 |
| Robin_Watts | I have now, and pkg-config --exists libcrypto still gives me nothing. | 18:23.12 |
| | oh, it's a return code thing. gah. | 18:23.34 |
| | I was hoping for a path to be printed. | 18:23.39 |
| sebras | it is. | 18:23.42 |
| | pkg-config --cflags libcrypto migth help? | 18:23.50 |
| | or --debug for the adventurous. | 18:24.14 |
| Robin_Watts | S'OK, found it now, thanks. | 18:24.44 |
| | OK, now it fails to find any of the openssl header files. | 18:27.31 |
| | or at least, is failing to find BIO_meth_.... | 18:27.51 |
| | sebras: I've pushed a new version with the fixes in, I hope. | 18:29.21 |
| sebras | takes a look. | 18:32.53 |
| | Robin_Watts: with that new version it compiles without warnings. | 18:34.05 |
| Robin_Watts | sebras: Fab. | 18:34.14 |
| | So, I just need a lgtm and I can push it. | 18:34.27 |
| | It must be very late where you are. Don't feel obliged to stay up for this! | 18:34.41 |
| sebras | Robin_Watts: a bit, but not too bad. | 18:34.56 |
| | Robin_Watts: in pdf_set_str_len() becuase newlen is now size_t we no longer needs to check for < 0 and I think the cast of newlen to (unsigned int) can now be removed, right? | 18:36.59 |
| Robin_Watts | yup (to both). will fix. | 18:38.09 |
| sebras | Robin_Watts: in pdf_format_key() we call pdf_sprint_obj() with int n as size_t cap and size_t *t as len. | 18:41.11 |
| | Robin_Watts: shouldn't int n in the declaration really be size_t n? | 18:41.23 |
| | but that would change the function pointers in fz_store_type. | 18:41.40 |
| Robin_Watts | that would indeed change the function pointers in fz_store_type. | 18:42.09 |
| sebras | I think that would be more correct, but that makes the patch larger, and you might think it is worth it? | 18:42.10 |
| Robin_Watts | Which may be OK, but... what you just said :) | 18:42.19 |
| | Let me try it and see how far that balloons. | 18:42.40 |
| sebras | it would definitely touch all format_key() callbacks. | 18:42.58 |
| Robin_Watts | yeah, but there are only 2 of them. | 18:43.42 |
| | 4 of them. | 18:44.13 |
| | loads of the buggers :) | 18:44.44 |
| | but all tiny changes. will add to the commit. | 18:45.26 |
| sebras | good to hear. | 18:47.58 |
| | hm.. perhaps we should change | 18:48.08 |
| | the type of len in PDFObject_asByteString() to size_t. | 18:48.37 |
| | all other places where we call pdf_to_str_len() we actually size_t already. (except for pdf_to_str_len() but that would then balloon into mujs). | 18:49.26 |
| Robin_Watts | ok, pushed with the tiny changes. | 18:50.23 |
| | Let me look at your new thing... | 18:50.30 |
| | yes. | 18:51.42 |
| | I can't parse that last sentence. | 18:52.10 |
| | Well, I can parse it, I can't grok it. | 18:52.23 |
| sebras | the only problem is that NewByteArray() then takes a jsize, which surprisingly is typedef jint jsize; which is typedef int jint;.... | 18:52.32 |
| | Robin_Watts: I pasted the wrong thing! | 18:52.54 |
| | ffi_PDFObject_valueOf() also calls pdf_to_str_len() but changing ffi_PDFObject_valueOf() would mean changing mujs. | 18:53.28 |
| | so probably not worth it. | 18:53.36 |
| Robin_Watts | I'm lost now. | 18:53.56 |
| | what were you suggesting changing that isn't worth changing? | 18:54.07 |
| | oh, just that last thing, right. | 18:54.38 |
| sebras | ffi_PDFObject_valueOf() calls pdf_to_str_len() and casts its return value to int. | 18:54.45 |
| | but I think fixing that might not be worth it because it would mean extra checks in murun or changing the functions in mujs as well. | 18:55.11 |
| Robin_Watts | sebras: jsize probably varies in size depending on 32 or 64 bit builds? | 18:55.16 |
| | no, jsize is jint which is "long". | 18:56.51 |
| sebras | http://cr.openjdk.java.net/~ngmr/vmi.00/html/jni__md_8h.html#ad6e59d976b9b5bae7a19d746bd2879c1 | 18:57.12 |
| Robin_Watts | We'd need jlong which is 64bit. | 18:57.13 |
| sebras | seems to depend on who you ask. | 18:57.18 |
| Robin_Watts | yeah, the docs I was looking at was oracle. | 18:57.38 |
| sebras | in IBMs java jsize is jlong which they clearly stated was 64 bits. | 18:57.45 |
| Robin_Watts | in any event it's a mess, and let's not touch it. | 18:57.49 |
| sebras | nope. | 18:57.52 |
| Robin_Watts | So, let me cluster that one last time... | 18:58.21 |
| sebras | Robin_Watts: pdf_set_text_field_value() sets selStart/selEnd to -1. | 18:58.33 |
| | and pdf_js_event_result_keystroke() sets selStart as an int. | 18:59.32 |
| | same for selEnd of course. | 18:59.40 |
| Robin_Watts | Well, the mujs stuff is "safe". | 19:00.31 |
| | I propose we change -1 to be (size_t)-1 | 19:00.46 |
| | So people can still check against the "special value", but they need to do so explicitly. | 19:01.17 |
| sebras | if we do that won't we change how selStart/selEnd are interpreted in source/pdf/js/util.js.h ? | 19:01.24 |
| | there is a checl if event.selStart >= 0 there. | 19:01.43 |
| Robin_Watts | Gah. Cos of the stupid one.c thing, vs doesn't pick that up on searches :( | 19:02.30 |
| sebras | oh, vs doesn't index any generated files? | 19:03.13 |
| Robin_Watts | Not when I search "Entire Solution" it seems. | 19:03.42 |
| sebras | Robin_Watts: I think the .h file in this case comes from source/pdf/js/util.js but since that is in javascript perhaps vs doesn't know that selStart in that language is the same as in C. | 19:04.24 |
| | or at least related. | 19:04.29 |
| Robin_Watts | mujs is going to fall in a heap for any size > INTMAX though. | 19:05.29 |
| | so let's just cast it to an int for that test. | 19:05.48 |
| sebras | Robin_Watts: it probably would, yes. | 19:06.11 |
| | Robin_Watts: how are int64_t and size_t related? | 19:10.56 |
| Robin_Watts | int64_t is always at least 64bits. | 19:11.07 |
| | signed. | 19:11.11 |
| sebras | Robin_Watts: and size_t is able to represent the larges possible object, right? | 19:11.25 |
| | i.e. whatever is the maximum value sizeof() can return. | 19:11.35 |
| Robin_Watts | size_t is an unsigned int at least as big as ptrdiff_t | 19:11.39 |
| | size_t is to do with the largest block of memory you can have. | 19:12.07 |
| | hence 32bit on 32bit systems, 64 on 64bit systems. | 19:12.27 |
| sebras | in complete_signatures() we now call pdf_array_push_int() which takes int64_t, but we give it size_t. | 19:12.44 |
| | not that I think we'll ever have values > 2**63-1. :) | 19:13.27 |
| | I'm surprised vs doesn't complain about that signedness thing though. | 19:13.45 |
| Robin_Watts | so size_t will be cast to int64_t, which is fine until someone figures out how to make a PDF file that allocates more than 2^63 bytes. | 19:13.57 |
| | oh, wait.... | 19:14.26 |
| sebras | for what? | 19:14.51 |
| Robin_Watts | in util.js, where we have "if (event.selStart >= 0)", that's js code. | 19:15.35 |
| | not C. | 19:15.40 |
| sebras | it is. | 19:15.40 |
| | nope. | 19:15.42 |
| Robin_Watts | It's javascript code, put into a block for compilation into C. It is not C code. | 19:16.23 |
| sebras | correct. | 19:16.34 |
| | that is an apt observation. | 19:16.44 |
| Robin_Watts | So, event.selStart is checking the thing that is set up by pdf_js_event_result_keystroke | 19:17.25 |
| sebras | yes. | 19:18.20 |
| Robin_Watts | No, sorry, it's pdf_js_event_init_keystroke | 19:18.30 |
| | That takes the event->selStart (size_t), casts it to a double, and puts that into the js value. | 19:19.05 |
| sebras | Robin_Watts: well. pdf_js_event_init_keystroke() sets it up for the javascript to use, and after the event handle has returned I'd assume that pdf_js_event_result_keystroke() runs and if rc is != 0 then selStart/selEnd are updated in C-world depending on what changes happened in js | 19:20.06 |
| Robin_Watts | so (size_t)-1 will give it conniptions. | 19:20.10 |
| | js_pushnumber(js->imp, (int64_t)event->selStart); will be safer. | 19:20.38 |
| | That will make it signed, then it'll convert to double without loss (in all the cases we care about). | 19:20.55 |
| | js_tryinteger will bugger it up on the way out of course... | 19:21.55 |
| sebras | Robin_Watts: what caused you to change selStart/selEnd to size_t? | 19:22.17 |
| Robin_Watts | you did :) | 19:22.26 |
| sebras | Robin_Watts: perhaps that is easier fix in another way? | 19:22.31 |
| | Robin_Watts: I did? | 19:22.59 |
| Robin_Watts | It's a knockon effect of pdf_to_str_len | 19:23.31 |
| sebras | Robin_Watts: is it? I can't find that pdf_to_str_len() called anywhere related to selStart/selEnd..? | 19:24.39 |
| Robin_Watts | sebras: And now, nor can I. | 19:24.50 |
| sebras | Robin_Watts: I think selStart/selEnd originated in a 64-bit warning in vs. | 19:26.13 |
| | Robin_Watts: but I don't know where it was. perhaps there is another solution to that that doesn't involve mujs? | 19:26.35 |
| Robin_Watts | rebuilding now, we'll see... | 19:26.43 |
| | OK, this builds OK, and has the selStart/selEnd stuff removed. | 19:29.33 |
| | Let me clustertest. | 19:29.43 |
| sebras | Robin_Watts: ok. | 19:29.51 |
| Robin_Watts | Oh, I hit the 500 of everything mark, and I'm about 4 days away from having every upgrade. Curse you. | 19:30.48 |
| sebras | Robin_Watts: :) | 19:31.02 |
| | Robin_Watts: I wonder what happens then. | 19:31.08 |
| | copystream | 19:31.37 |
| Robin_Watts | Well, it'll take another year to get level 10 of everything which is required to get every achievement. | 19:31.51 |
| sebras | in copystream() we have a size_t len which we cast to int before passing it to pdf_encrypted_len() which now takes a size_t. | 19:32.39 |
| | pdf_encrypted_len() also returns size_t which we supply into a pdf_dict_put_int() that takes int64_t. presumably this is not a problem. | 19:33.05 |
| | but the initial cast to int ought to be possible to remove..? | 19:33.17 |
| Robin_Watts | (int) -> (int64_t) I think. | 19:33.46 |
| sebras | why? | 19:33.57 |
| | len is already size_t, just pass it as is? | 19:34.06 |
| Robin_Watts | no, sorry, was misreading. | 19:34.08 |
| | pdf_dict_put_int takes an int64_t. | 19:34.23 |
| sebras | and if pdf_encrypted_len() returns > 2**63-1 we're in trouble for other reasons. | 19:34.44 |
| Robin_Watts | so we're getting away with implicitly casting a sign change. | 19:34.45 |
| sebras | Robin_Watts: I don't understand what you mean. we're casting the argument to pdf_encrypted_len(), not its return value. | 19:35.55 |
| Robin_Watts | pdf_encrypted_len returns a size_t. | 19:36.11 |
| sebras | how does adding a cast help us? | 19:36.12 |
| Robin_Watts | which is passed as an int64_t. | 19:36.18 |
| | no one has spotted a compiler whining about this, so hence "we've been getting away with it". | 19:36.47 |
| | If compilers aren't going to care, I don't either. | 19:37.58 |
| | New version pushed. | 19:39.40 |
| sebras | Robin_Watts: it is fun that we change pdf_add_codespace() to take size_t because the size of the lexing buffer is size_t. I wonder if we won't croak way earlier if we encounter a cmap string > 2**32 chars. | 19:45.16 |
| Robin_Watts | oh, I'm sure we will :) | 19:47.51 |
| | latest version cleared the cluster. | 19:48.34 |
| sebras | if you change fz_write_base64() you ought to update docs/api/io.html | 19:57.52 |
| Robin_Watts | done. | 20:00.03 |
| | sebras: Any second now, I'm going to be called away by Helen. | 20:01.00 |
| sebras | Robin_Watts: do we need to expclitly cast the -1 in pdf_process_stream() when assigning cookie->progress_max? | 20:01.03 |
| | yeah, but this is likely my last question! :) | 20:01.18 |
| Robin_Watts | I don't think we need to cast at assignment time. | 20:01.25 |
| | no compiler has complained. | 20:01.29 |
| sebras | let me try clang for good measure. | 20:02.11 |
| Robin_Watts | just 1 sec... | 20:02.26 |
| | ok, new version. | 20:02.51 |
| sebras | Robin_Watts: clang doesnt' complain either. | 20:03.26 |
| Robin_Watts | Fab. | 20:03.31 |
| | so, if you're happy, I'll push that in a mo. | 20:03.39 |
| sebras | fetches. | 20:03.39 |
| | Robin_Watts: LGTM "Fix various APIs to use size_t's for lengths." | 20:04.51 |
| Robin_Watts | Thanks. | 20:04.57 |
| sebras | I haven't looked at the pdf_walk_tree onwards. that will be tomorrow. | 20:05.03 |
| Robin_Watts | tor looked at that, and paul at the next one. | 20:05.13 |
| | I have lgtm's for them already. | 20:05.20 |
| sebras | ok. | 20:05.27 |
| Robin_Watts | so no need to trouble yourself. Thanks again for this. | 20:05.45 |
| sebras | Robin_Watts: sorry to be a PITA, but I hope that the commit is even better now. :) | 20:05.50 |
| Robin_Watts | No problem - much appreciated. This is why I like code reviews! | 20:06.11 |
| | (and git) | 20:06.14 |
| sebras | I specifically like rebase. | 20:06.22 |
| Robin_Watts | yeah. | 20:06.26 |
| sebras | now I'll go to bed. see ya! | 20:06.44 |
| Robin_Watts | I like the ability to like to world, and rewrite my mistakes out of existence. | 20:06.47 |
| | night. | 20:06.49 |
| | s/like to/lie to the/ | 20:07.04 |
| malc_ | sebras: https://stackoverflow.com/questions/10168079/why-is-size-t-unsigned | 20:09.23 |
| | <<<Back 1 day (to 2020/01/12) | Forward 1 day (to 2020/01/14)>>> | |
Log of #mupdf at irc.freenode.net.