Re: [Off-Topic] 1st proposal for GMime error reporting

[Jeffrey Stedfast <jestedfa microsoft com>]

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ß" <albrecht dress arcor de> 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.