Hi Albrecht, Yea, I was thinking that we'd probably have to ultimately add functionality to GMimeParserOptions as well (because not all of the issues described in the slide deck can be done directly inside of GMimeParser as you've noted), just wasn't sure if I'd want the user to use it directly or use some sort of signal handler on GMimeParser (where the options would somehow proxy it back to the parser).
I think, though, that proxying stuff back to the GMimeParser would get ugly quickly, so it probably does make more sense to have a callback on GMimeParserOptions like you've done. I think your patch seems reasonable for a start. I would prefer "Callback" over "CB" to be consistent with glib/gtk naming conventions and maybe use the terms "Warn/Warning" instead of "Issue/Notify/Error", but those are minor nitpicks ;-) As far as the actual logic of the patch goes, I noticed that when a header exists already, you don't still add the header anyway. I couldn't tell at a glance whether you did the same with parameters, but I don't think that this patch should change behavior - it should only add a mechanism for alerting the application of potential issues that may arise when interpreting the MIME. Now, *perhaps*, the callback could return some sort of value to the inner workings of the parser logic that would determine if the header/parameter/etc should be dropped, but it shouldn't happen without application feedback because that would alter the message, breaking the work I did to make sure that parsing a message and then writing it back to a stream should produce an exact byte-for-byte copy of the original message stream by default. The only gotchas that I can think of with regards to a return value from the callback is that the callback may need more context than what we can easily provide right now and that different issues/warnings may require different sets of return values (which may make some return values have undefined behavior depending on the context). We might need to try to figure out what sort of actions make sense for each warning condition and see if they are consistent. (Note: what if you get multiple headers or parameters but want to only keep the last one found rather than the first?) Another thing that I just thought of is that I've noticed that some clients with include the same parameters 2 times, once encoded using rfc2231 and once using the (incorrect-but-widely-used-anyway) rfc2047 encoding. In the "valid" cases, these values will be identical, it's just that the sending client is trying to be "helpful" if the receiving client doesn't support one form of encoding or the other. To be honest, I'm not sure this is really all that helpful, but I digress ;-) Just something to be aware of... Anyway, I look forward to seeing your next patch. If you have a github account, using a PR would be really helpful too (if you don't have one, it's not a huge deal). Jeff On 10/21/17, 1:34 PM, "Albrecht Dreß" <[email protected]> wrote: Ummm… I erased the gzip'ed message before sending, which for some strange reason invalidated the signature. 2nd try, with both parts ijn a tar… Sorry for the confusion, Albrecht. Am 21.10.17 19:20 schrieb(en) Albrecht Dreß: > Hi Jeff: > > In the meantime, I looked into ways to implement the GMime parser error reporting feature. Instead of using a signal emitted by the GMimeParser object, I think it might be better to add a “issue callback” to GMimeParserOptions, as it is also used by many other functions. > > The attached little patch implements a /very/ basic feasibility demonstration: > - add the callback to GMimeParserOptions; > - call it from the parser if a duplicated “Content-*” header is detected, and drop the duplicated header; > - check for duplicated header parameters in decode_param_list() and keep only the first one. > > Note that for the latter, it is not possible to report the originating object and parser offset, as it is not passed downstream into the function. Do you have an idea how this could be achieved, without adding extra parameters to all the functions? Maybe add an internal reference to the parser object to GMimeParserOptions? > > The possibility of removing “evil” headers or parameters should probably be configurable through GMimeParserOptions. If it is activated, it can be used to parse and re-write a sanitised message with a well-known behaviour. > > Running the attached suspicious message data block (packed, as Balsa would attach it as message/rfc822 otherwise…) through the modified test-parser application, all 6 issues are detected and reported. > > What do you think about this approach? Should I proceed this way? > > Cheers, Albrecht. _______________________________________________ balsa-list mailing list [email protected] https://mail.gnome.org/mailman/listinfo/balsa-list
