Review Request: sieve variables extension (dev/cassell/for-review-sieve-variables-rev1)

James Cassell fedoraproject at
Mon Jan 12 00:19:31 EST 2015


I believe I have a more or less complete implementation of RFC 5229, 
Sieve: Variables Extension.

tl;dr: I'd like help making sure that the sanity checks currently 
performed at script compile time are also performed at runtime for 
the actions (or tests) that require it.

At this point, I have cleaned up the patches, and I plan to drop the 
ones that haven't been cleaned up since they were mostly for 
debugging.  I'm open to any feedback if anyone would like the commits 
reworked in a particular way.

I plan to add some more CUnit tests to get some ongoing automated 
testing to make sure nothing breaks going forward, though I'm (as 
always) short on time.

One problem that I have thought of is that checks performed in the 
sieve parser (sieve/sieve.y) will possibly need to be performed at 
runtime if the variables extension is enabled.  These checks are in 
verify_utf8, verify_regex, verify_envelope, verify_addrheader, 
verify_header, verify_mailbox, and verify_address, in addition to any 
one-off checks in any individual rules.  It appears that this will 
apply mainly to actions, and possibly to the regex match-type.  It 
might also apply to what happens if an address or envelope test is 
applied to a non-address containing header, or if a test becomes 
malformed.  In such a case, should we have the test return false, or
should we abort the script and fallback to the implicit keep?  The 
thing I hate about runtime errors is that there is no notification to 
the script author that they happened.

Updates from last time:

On Tue, Nov 11, 2014, at 11:12 AM, James Cassell wrote:
> What works:
> - setting variables using the SET action
> - match variables are set when the :matches match-type is used (${0},
> ${1}, etc)
>   - did this by overloading the comprock, which had been null for
>   :matches test
>     - should I do this differently?
> - most of the places that you would normally use a string, you can also
> add variables to your strings

This all still works.

> What doesn't work:
> - modifiers to the set action


> - the STRING test


> - :regex match-type
>   - Ken, I might need your help on this one

I don't think I'll get to this one and I don't consider it crucial 
since its not an official RFC.

> What I don't know how to do:
> - evaluate for UTF-8 support

I still have no idea what I should be looking for here.

> What I'd like reviewed:
> - the code
> - the above stuff that I don't know
> - security issues due to VARIABLES interaction with sieve actions
> - anything else you can think of

Still applies

> Also, I believe the first five patches are acceptable for master.  (I
> won't push them, though, until I've given people a chance to look them
> over if they want.)

At this point, I'll likely just push them when merging variables to 

I look forward to any feedback!


James Cassell

The following changes since commit e1ff81794b435586d640fa41e16a285f8bccdeb9:

  [doc] clarify how to write CUnit tests (2015-01-02 21:21:53 -0500)

are available in the git repository at:

  git:// dev/cassell/for-review-sieve-variables-rev1

for you to fetch changes up to 099c7abf2bf98ea8ce9fa44c6f93585587e6734d:

  [sieve] grammar.c: check attempted use of namespaced variables (2015-01-09 17:32:04 -0500)

James Cassell (50):
      [sieve] bc_eval.c: allow {add,set,remove}flag actions with reject
      [sieve] change imap4flags variable names to support VARIABLES extension
      [sieve] use central variable_list_t to hold action flag lists
      [sieve] bc_eval.c: do all imap4flags processing in bc_eval.c
      [sieve] remove do_{{add,set,remove}flag,{un,}mark}() functions
      [sieve] sieve.y: define tokens for VARIABLES extension
      [sieve] sieve-lex.l: teach lexer VARIABLES tokens
      [sieve] add VARIABLES reference
      [sieve] enable awareness of the variables extension
      [sieve] add parser support for string test
      [sieve] add parser support for SET action
      [sieve] bytecode.h: add codepoints required by variables base spec
      [sieve] bc_generate.c: variables base spec support
      [sieve] bc_dump.c: variables base spec support
      [sieve] bc_emit.c: variables base spec support
      sieve test script
      [sieve] sieved.c: variables base spec support
      [sieve] store require'd extensions in the bytecode
      [sieve] implement parse_string() to resolve variables in strings
      [sieve] special variables for storing match vars and parsed strings
      [sieve] implement variables SET action
      debug printf
      [sieve] bc_eval.c: pass variables to eval_bc_test()
      [sieve] add support for match variables to :matches match-type
      [sieve] comparator.c: *_matches(): only use given void *rock if non-NULL
      [sieve] grammar.c: parse_string(): Allow parsing of match variables
      [sieve] bc_eval.c: parse strings for variables when reading bytecode
      [sieve] bc_eval.c: attempt proper variables support in imap4flags
      break line to avoid horiz scrolling
      BC_STRING maps to BC_HASFLAG
      test script changes
      [sieve] bc_eval.c: implement variables STRING test
      update for debugging
      [sieve] sieve.y: support variables in HASFLAG test
      [sieve] bc_eval.c: support non-existent variable in hasflag test
      [sieve] variables.[ch]: template for SET action modifiers
      [sieve] variables.c: allocate buffer for resultant modified string
      [sieve] bc_eval.c: body test should not set match variables
      [sieve] variables.c: implement and test set action modifiers
      [cunit] sieve: :quotewildcard with :encodeurl in variable_modifiers
      [sieve] bc_eval.c: apply specified variable modifiers in set action
      [sieve] support variables extension in setflag action
      [sieve] parser support for {add,remove}flag actions with variables
      [sieve] support variables extension in {add,remove}flag actions
      [sieve] bc_eval.c: plan support for :count with variables in tests
      [sieve] support :count with variables in hasflag and string tests
      [sieve] bc_eval.c: break from loop after calculating :count
      [sieve] flags.[ch]: properly use EXPORTED
      [sieve] grammar.c: check attempted use of namespaced variables                                        |   4 +
 cunit/sieve.testc                                  |  79 +++-
 doc/specs.html                                     |   2 +
 lib/imapoptions                                    |   2 +-
 sieve/README                                       |   3 +
 sieve/bc_dump.c                                    |  59 ++-
 sieve/bc_emit.c                                    |  84 +++-
 sieve/bc_eval.c                                    | 517 ++++++++++++++++++---
 sieve/bc_generate.c                                | 105 ++++-
 sieve/bytecode.h                                   |  34 +-
 sieve/comparator.c                                 |  96 +++-
 sieve/flags.c                                      |   4 +
 sieve/flags.h                                      |   2 +-
 sieve/grammar.c                                    | 167 +++++++
 sieve/grammar.h                                    |  18 +
 sieve/message.c                                    |  84 ----
 sieve/script.c                                     |  67 +--
 sieve/script.h                                     |   1 +
 sieve/sieve-lex.l                                  |  16 +-
 sieve/sieve.y                                      | 309 ++++++++++--
 sieve/sieve_interface.h                            |   1 +
 sieve/sieved.c                                     |  63 ++-
 .../actionExtensions/uberExtensionActionScript.s   |  76 +--
 .../tests/testExtension/uberExtensionTestScript.s  |  59 ++-
 sieve/tree.c                                       |   9 +-
 sieve/tree.h                                       |  12 +-
 sieve/variables.c                                  | 197 ++++++++
 sieve/variables.h                                  |  13 +
 sieve/varlist.h                                    |   2 +
 29 files changed, 1724 insertions(+), 361 deletions(-)
 create mode 100644 sieve/grammar.c
 create mode 100644 sieve/grammar.h
 create mode 100644 sieve/variables.c
 create mode 100644 sieve/variables.h

More information about the Cyrus-devel mailing list