I could not check all the areas Rich listed above(although I did find other areas not listed that needed to be changed):

DS_LASGroupDnEval -> the filter is generated by slapi_ldap_url_parse -> ldap_url_parse(_ext)

I do NOT check full filters in this fix, just the values we add to filters.

The function escape_filter_value() is currently used primarily for debugging output, where it doesn't matter if the string is truncated at BUFSIZ. For general purpose use, we need to be able to handle filters larger than BUFSIZ i.e. we need to be able to allocate the memory and return allocated memory to the caller, and have the caller free that memory. Similar to the way it works in windows_protocol_util.c:find_entry_by_attr_value() - we just assume every character will be escaped, so len * 3. openldap ldap_bv2escaped_filter_value_x() is actually smarter than that - it will pre-scan the string to figure out how long the escaped filter will be, then allocate that much memory if necessary.

Ideally we would have a function that works like slapi_ch_smprintf but smarter. You could pass in a buffer to use, and if the result would overflow that buffer, return malloc'd memory instead. In most cases, filters will be small, so we should allow small fixed size buffers to be used, but we also need to handle the "abnormal" cases (or attacks) where someone is passing a very, very large filter that requires escaping.

1) It's just a coding style issue, but we usually use slapi_ch_free_string to free a string (char ) to avoid cast (void ). We'd like to get rid of cast as much as we can... Also, slapi_ch_free* takes a pointer to NULL. So, we don't need "if (filter_str_ptr)"...
2979 if (new_filter_str) slapi_ch_free((void ) &new_filter_str);
2970 if (filter_str_ptr) slapi_ch_free((void *) &filter_str_ptr);

2) It looks slapi_escape_filter_value has a chance to return NULL. If it occurs, strlen crashes.
539 escaped_filter_val = slapi_escape_filter_value(dn, dn_len);
540 dn_len = strlen(escaped_filter_val);

3) I guess in this function get_substring_filter, val returned from slapi_escape_filter_value is leaked?

4) I'm just curious. Why these values are string instead of an integer value?
3577 #define ESC_NEXT_VAL "ESC__NEXT__VAL"
3578 #define NORM_NEXT_VAL "NORM__NEXT__VAL"
3579 #define ESC_AND_NORM_NEXT_VAL "ESC__AND__NORM__NEXT__VAL"

Can it be like this?

define ESC_NEXT_VAL 1

escaped_val = slapi_filter_sprintf("cn=%d%s", ESC_NEXT_VAL, value);

If it's possible, comparing 2 integer values would be easier/faster than comparing 2 strings...
if (strcmp(val, ESC_NEXT_VAL) == 0){

5) slapi_sdn_get_dn returns normalized DN. In addition to it, slapi_sdn_get_ndn converts upper case chars to lower case. We have to call slapi_sdn_get_dn since we don't want to lose the case information in the DN values.
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_dn(sdn));
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_ndn(sdn));

Replying to [comment:6 nhosoi]:

1) It's just a coding style issue, but we usually use slapi_ch_free_string to free a string (char ) to avoid cast (void ). We'd like to get rid of cast as much as we can... Also, slapi_ch_free* takes a pointer to NULL. So, we don't need "if (filter_str_ptr)"...
2979 if (new_filter_str) slapi_ch_free((void ) &new_filter_str);
2970 if (filter_str_ptr) slapi_ch_free((void *) &filter_str_ptr);

No problem.

2) It looks slapi_escape_filter_value has a chance to return NULL. If it occurs, strlen crashes.
539 escaped_filter_val = slapi_escape_filter_value(dn, dn_len);
540 dn_len = strlen(escaped_filter_val);

I'll check this out.

3) I guess in this function get_substring_filter, val returned from slapi_escape_filter_value is leaked?

I'll look into this.

4) I'm just curious. Why these values are string instead of an integer value?
3577 #define ESC_NEXT_VAL "ESC__NEXT__VAL"
3578 #define NORM_NEXT_VAL "NORM__NEXT__VAL"
3579 #define ESC_AND_NORM_NEXT_VAL "ESC__AND__NORM__NEXT__VAL"

Can it be like this?

define ESC_NEXT_VAL 1

escaped_val = slapi_filter_sprintf("cn=%d%s", ESC_NEXT_VAL, value);

If it's possible, comparing 2 integer values would be easier/faster than comparing 2 strings...
if (strcmp(val, ESC_NEXT_VAL) == 0){

Great idea, I'll work on this.

5) slapi_sdn_get_dn returns normalized DN. In addition to it, slapi_sdn_get_ndn converts upper case chars to lower case. We have to call slapi_sdn_get_dn since we don't want to lose the case information in the DN values.
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_dn(sdn));
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_ndn(sdn));

In my testing, the value was NEVER normalized until I changed this to use the "ndn" function. This was because the sdn did not have the normalized dn in its struct(its NULL). So are you saying if the slapi_DN doesn't have the normalized dn in its structure, or just return the unnormalized dn? In effect we don't normalize anything, we only return a normalized DN if one exists. I thought Rich wanted to "normalize" a value, maybe not.

Replying to [comment:7 mreynolds]:

Replying to [comment:6 nhosoi]:

5) slapi_sdn_get_dn returns normalized DN. In addition to it, slapi_sdn_get_ndn converts upper case chars to lower case. We have to call slapi_sdn_get_dn since we don't want to lose the case information in the DN values.
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_dn(sdn));
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_ndn(sdn));

In my testing, the value was NEVER normalized until I changed this to use the "ndn" function.

If that's the symptom, that's a bug...

value_normalize_value passes in value->bv.bv_val to sdn. In the function, udn, dn and ndn in sdn are initialized to NULL. Then, value->bv.bv_val is set to udn. In slapi_sdn_get_dn, since dn and ndn are both NULL, udn would be normalized and set to dn and the dn is returned.
value_normalize_value(Slapi_Value *value)

Please see slapi_sdn_get_ndn in dn.c, it just calls slapi_sdn_get_dn internally, lower-case the string, then return it...

This was because the sdn did not have the normalized dn in its struct(its NULL).

Again, if sdn does not have the normalize dn (NULL), slapi_sdn_get_dn is supposed to call slapi_dn_normalize_ext and generate the normalized dn. If you see something else, that's a bug. Please open a ticket (with a reproducer ;).

So are you saying if the slapi_DN doesn't have the normalized dn in its structure, or just return the unnormalized dn? In effect we don't normalize anything, we only return a normalized DN if one exists. I thought Rich wanted to "normalize" a value, maybe not.

Replying to [comment:8 nhosoi]:

Replying to [comment:7 mreynolds]:

Replying to [comment:6 nhosoi]:

5) slapi_sdn_get_dn returns normalized DN. In addition to it, slapi_sdn_get_ndn converts upper case chars to lower case. We have to call slapi_sdn_get_dn since we don't want to lose the case information in the DN values.
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_dn(sdn));
595 value->bv.bv_val = slapi_ch_strdup(slapi_sdn_get_ndn(sdn));

In my testing, the value was NEVER normalized until I changed this to use the "ndn" function.

If that's the symptom, that's a bug...

value_normalize_value passes in value->bv.bv_val to sdn. In the function, udn, dn and ndn in sdn are initialized to NULL. Then, value->bv.bv_val is set to udn. In slapi_sdn_get_dn, since dn and ndn are both NULL, udn would be normalized and set to dn and the dn is returned.
value_normalize_value(Slapi_Value *value)

Please see slapi_sdn_get_ndn in dn.c, it just calls slapi_sdn_get_dn internally, lower-case the string, then return it...

This was because the sdn did not have the normalized dn in its struct(its NULL).

Again, if sdn does not have the normalize dn (NULL), slapi_sdn_get_dn is supposed to call slapi_dn_normalize_ext and generate the normalized dn. If you see something else, that's a bug. Please open a ticket (with a reproducer ;).

So are you saying if the slapi_DN doesn't have the normalized dn in its structure, or just return the unnormalized dn? In effect we don't normalize anything, we only return a normalized DN if one exists. I thought Rich wanted to "normalize" a value, maybe not.

Take a look at the implementation for slapi_sdn_get_dn - if dn or ndn is set, they are returned. If not, then it normalizes udn and returns the normalized dn. Since value_normalize_value() calls sdn = slapi_sdn_new_dn_passin(value->bv.bv_val); this sets the udn in the sdn, so slapi_sdn_get_dn should normalize the value and return ndn.

There is no benefit for using an "int" for:

define ESC_NEXT_VAL

define NORM_NEXT_VAL

define ESC_AND_NORM_NEXT_VAL

PR_vsxprintf(func, ..., ...);

func always uses a const char for the value, so we will always have to use strcmp().

For the normalization stuff, you(both) are right, I will use slapi_sdn_get_dn().

Does filter_stuff_func ensure that the string is null terminated?

Does it always work to escape before normalize? Is it possible that escaping the value first will inject characters into the string that will cause normalization to fail?

{{{
value_normalize_value(v);
}}}
This doesn't normalize anything except DNs. You'll have to use slapi_attr_value_normalize_ext() instead.

{{{
extra_space = (ctx->buf_len + filter_len + 16);
}}}
Why 16?

{{{
if(ldap_bv2escaped_filter_value(&raw_filter, &escaped_filter) != 0){
LDAPDebug(LDAP_DEBUG_TRACE, "slapi_filter_sprintf: failed to escape filter value(%s)\n",val,0,0);
return -1;
}}}
You need to ctx->next_arg_needs_esc_norm = 0; and in the mozldap case too. You also need to slapi_ch_free_string(&buf); in the mozldap case.

I don't know how safe or portable this is:
{{{
char do_escape_string (const char str, int len, char buf[BUFSIZ], int (special)(unsigned char));
...
buf = slapi_ch_calloc(sizeof(char), slen3 + 1);
do_escape_string(val, slen, buf, special_filter);
}}}
Is it safe/portable to pass in a malloced string to a function that expects a fixed size buffer, especially when we write past BUFSIZ into buf? I wrote a small test program to write more than BUFSIZ bytes to the string, with stack buffers on either side, and it did not overwrite the stack anywhere I could see. This was on RHEL6.3 with gcc-4.4.6-4.el6.x86_64. But this does seem strange and I don't know what the C specs say nor what other compiler versions will do. As long as we use openldap though it is not an issue.

In lookup_instance_name_by_suffix() - in the exact case, you only escape the value used in the quotes, but escape and normalize the unquoted search string - but in the not exact case, you escape and normalize the value used with the quote, but only escape the unquoted value - why?

Replying to [comment:11 rmeggins]:

Does filter_stuff_func ensure that the string is null terminated?

Does it always work to escape before normalize? Is it possible that escaping the value first will inject characters into the string that will cause normalization to fail?
Good question, but I figured it was best to escape and then normalize, as opposed to doing the opposite. I'll have to test this.

{{{
value_normalize_value(v);
}}}
This doesn't normalize anything except DNs. You'll have to use slapi_attr_value_normalize_ext() instead.

{{{
extra_space = (ctx->buf_len + filter_len + 16);
}}}
Why 16?

I was just trying to increase the buffer by a little bit.

{{{
if(ldap_bv2escaped_filter_value(&raw_filter, &escaped_filter) != 0){
LDAPDebug(LDAP_DEBUG_TRACE, "slapi_filter_sprintf: failed to escape filter value(%s)\n",val,0,0);
return -1;
}}}
You need to ctx->next_arg_needs_esc_norm = 0; and in the mozldap case too.
ok

You also need to slapi_ch_free_string(&buf); in the mozldap case.

buf always gets freed by the caller.

I don't know how safe or portable this is:
{{{
char do_escape_string (const char str, int len, char buf[BUFSIZ], int (special)(unsigned char));
...
buf = slapi_ch_calloc(sizeof(char), slen3 + 1);
do_escape_string(val, slen, buf, special_filter);
}}}
Is it safe/portable to pass in a malloced string to a function that expects a fixed size buffer, especially when we write past BUFSIZ into buf? I wrote a small test program to write more than BUFSIZ bytes to the string, with stack buffers on either side, and it did not overwrite the stack anywhere I could see. This was on RHEL6.3 with gcc-4.4.6-4.el6.x86_64. But this does seem strange and I don't know what the C specs say nor what other compiler versions will do. As long as we use openldap though it is not an issue.

This is exactly what you did in windows_protocol_util.c, so I figured it was ok.

In lookup_instance_name_by_suffix() - in the exact case, you only escape the value used in the quotes, but escape and normalize the unquoted search string - but in the not exact case, you escape and normalize the value used with the quote, but only escape the unquoted value - why?

To be honest, I don't recall why I did it that way. Maybe I was worried about the quotes. Simply looks like a mistake on my part.

Replying to [comment:12 mreynolds]:

Replying to [comment:11 rmeggins]:

Does filter_stuff_func ensure that the string is null terminated?

Does it always work to escape before normalize? Is it possible that escaping the value first will inject characters into the string that will cause normalization to fail?
Good question, but I figured it was best to escape and then normalize, as opposed to doing the opposite. I'll have to test this.

Ok.

{{{
value_normalize_value(v);
}}}
This doesn't normalize anything except DNs. You'll have to use slapi_attr_value_normalize_ext() instead.

{{{
extra_space = (ctx->buf_len + filter_len + 16);
}}}
Why 16?

I was just trying to increase the buffer by a little bit.

Ok.

{{{
if(ldap_bv2escaped_filter_value(&raw_filter, &escaped_filter) != 0){
LDAPDebug(LDAP_DEBUG_TRACE, "slapi_filter_sprintf: failed to escape filter value(%s)\n",val,0,0);
return -1;
}}}
You need to ctx->next_arg_needs_esc_norm = 0; and in the mozldap case too.
ok

You also need to slapi_ch_free_string(&buf); in the mozldap case.

buf always gets freed by the caller.

Ok.

I don't know how safe or portable this is:
{{{
char do_escape_string (const char str, int len, char buf[BUFSIZ], int (special)(unsigned char));
...
buf = slapi_ch_calloc(sizeof(char), slen3 + 1);
do_escape_string(val, slen, buf, special_filter);
}}}
Is it safe/portable to pass in a malloced string to a function that expects a fixed size buffer, especially when we write past BUFSIZ into buf? I wrote a small test program to write more than BUFSIZ bytes to the string, with stack buffers on either side, and it did not overwrite the stack anywhere I could see. This was on RHEL6.3 with gcc-4.4.6-4.el6.x86_64. But this does seem strange and I don't know what the C specs say nor what other compiler versions will do. As long as we use openldap though it is not an issue.

This is exactly what you did in windows_protocol_util.c, so I figured it was ok.

Ok. Then leave it as is.

In lookup_instance_name_by_suffix() - in the exact case, you only escape the value used in the quotes, but escape and normalize the unquoted search string - but in the not exact case, you escape and normalize the value used with the quote, but only escape the unquoted value - why?

To be honest, I don't recall why I did it that way. Maybe I was worried about the quotes. Simply looks like a mistake on my part.

New fix being sent out for review...

1.

{{{
diff --git a/ldap/servers/slapd/main.c b/ldap/servers/slapd/main.c index 7b15249..97822cd 100644
a b lookup_instance_name_by_suffix(char suffix,
1929 1929 goto done;
1930 1930
1931 1931 if (isexact) {
1932 query = slapi_ch_smprintf("(&(objectclass=nsmappingtree)(|(cn=\"%s\")(cn=%s)))", suffix, suffix);
1932 query = slapi_filter_sprintf("(&(objectclass=nsmappingtree)(|(cn=\"%s%s\")(cn=%s%s)))",
1933 ESC_NEXT_VAL, suffix, ESC_AND_NORM_NEXT_VAL, suffix);
1933 1934 if (query == NULL)
1934 1935 goto done;
1935 1936
… … lookup_instance_name_by_suffix(char suffix,
1951 1952 } else {
1952 1953 char suffixp = suffix;
1953 1954 while (NULL != suffixp && strlen(suffixp) > 0) {
1954 query = slapi_ch_smprintf("(&(objectclass=nsmappingtree)(|(cn=%s\")(cn=%s)))", suffixp, suffixp);
1955 query = slapi_filter_sprintf("(&(objectclass=nsmappingtree)(|(cn=\"%s%s\")(cn=%s%s)))",
1956 ESC_AND_NORM_NEXT_VAL, suffixp, ESC_NEXT_VAL, suffixp);
1955 1957 if (query == NULL)
1956 1958 goto done;
1957 1959 / Note: This DN is no need to be normalized. /
}}}
I'm curious ... In slapi_filter_sprintf at the line 1932, the first suffix in double quotes is just escaped, not normalized; the second suffix is escaped and normalized. While, in slapi_fileter_sprintf at the line 1955, suffixp in the double quotes is escaped and normalized, but the second one is just escaped. Is it intentional? Also, the slapi_filter_sprintf at the line 1955 isn't for substring match with ''? The original slapi_ch_smprintf has '*' in both "cn=", but I don't see them at the line 1955...

2.
Could you please define macros for, for instance, 256 and use the macro in the function which is referring attr? Someone may change one value and miss the other in the future...

{{{

define FILTER_CTX_ATTR_LEN 256 or something like that...

234 struct filter_ctx { 
235   char *buf; 
236   char attr[256];  <====
237   int attr_position; 
238   int attr_found; 
239   int buf_size; 
240   int buf_len; 
241   int next_arg_needs_esc_norm; 
242   int skip_escape; 
243 }; 
276     if(ctx->attr_found == 0 && ctx->attr_position < 255){  <====
277         if(ctx->attr[0] == '\0'){ 
278             if(strstr(val,"=")){ 
279                 /* we have an attr we need to record */ 
280                 if(!special_attr_char(val[0])){ 
281                     memcpy(ctx->attr, val, 1); 
282                     ctx->attr_position++; 
297                 if(special_attr_char(val[0])){ 
298                     /* this is not an attribute, we should not be collecting this, reset everything */ 
299                     memset(ctx->attr, '\0', 256);  <====
300                     ctx->attr_position = 0; 
369         ctx->attr_found = 0; 
370         ctx->attr_position = 0; 
371         memset(ctx->attr, '\0', 256);  <====
407 char* 
408 slapi_filter_sprintf(const char *fmt, ...) 
409 { 
410     struct filter_ctx ctx; 
411     va_list args; 
412     char *buf; 
413     int rc; 
414  
415     buf = slapi_ch_calloc(sizeof(char), 128 + 1);
416     ctx.buf = buf; 
417     memset(ctx.attr,'\0', 256);  <====
418     ctx.attr_position = 0; 
419     ctx.attr_found = 0; 
420     ctx.buf_len = 128;

}}}

358             extra_space = (ctx->buf_len + filter_len + 16);

And may I ask why '16'?

Replying to [comment:15 nhosoi]:

1.

{{{
diff --git a/ldap/servers/slapd/main.c b/ldap/servers/slapd/main.c index 7b15249..97822cd 100644
a b lookup_instance_name_by_suffix(char suffix,
1929 1929 goto done;
1930 1930
1931 1931 if (isexact) {
1932 query = slapi_ch_smprintf("(&(objectclass=nsmappingtree)(|(cn=\"%s\")(cn=%s)))", suffix, suffix);
1932 query = slapi_filter_sprintf("(&(objectclass=nsmappingtree)(|(cn=\"%s%s\")(cn=%s%s)))",
1933 ESC_NEXT_VAL, suffix, ESC_AND_NORM_NEXT_VAL, suffix);
1933 1934 if (query == NULL)
1934 1935 goto done;
1935 1936
… … lookup_instance_name_by_suffix(char suffix,
1951 1952 } else {
1952 1953 char suffixp = suffix;
1953 1954 while (NULL != suffixp && strlen(suffixp) > 0) {
1954 query = slapi_ch_smprintf("(&(objectclass=nsmappingtree)(|(cn=%s\")(cn=%s)))", suffixp, suffixp);
1955 query = slapi_filter_sprintf("(&(objectclass=nsmappingtree)(|(cn=\"%s%s\")(cn=%s%s)))",
1956 ESC_AND_NORM_NEXT_VAL, suffixp, ESC_NEXT_VAL, suffixp);
1955 1957 if (query == NULL)
1956 1958 goto done;
1957 1959 / Note: This DN is no need to be normalized. */
}}}
I'm curious ... In slapi_filter_sprintf at the line 1932, the first suffix in double quotes is just escaped, not normalized; the second suffix is escaped and normalized. While, in slapi_fileter_sprintf at the line 1955, suffixp in the double quotes is escaped and normalized, but the second one is just escaped. Is it intentional?

This was an accident, and they are now all just "escapes"

Also, the slapi_filter_sprintf at the line 1955 isn't for substring match with ''? The original slapi_ch_smprintf has '' in both "cn=", but I don't see them at the line 1955...

Nice catch.

2.
Could you please define macros for, for instance, 256 and use the macro in the function which is referring attr? Someone may change one value and miss the other in the future...

Done!

{{{

define FILTER_CTX_ATTR_LEN 256 or something like that...

234 struct filter_ctx {
235 char buf;
236 char attr[256]; <====
237 int attr_position;
238 int attr_found;
239 int buf_size;
240 int buf_len;
241 int next_arg_needs_esc_norm;
242 int skip_escape;
243 };
276 if(ctx->attr_found == 0 && ctx->attr_position < 255){ <====
277 if(ctx->attr[0] == '\0'){
278 if(strstr(val,"=")){
279 / we have an attr we need to record /
280 if(!special_attr_char(val[0])){
281 memcpy(ctx->attr, val, 1);
282 ctx->attr_position++;
297 if(special_attr_char(val[0])){
298 / this is not an attribute, we should not be collecting this, reset everything /
299 memset(ctx->attr, '\0', 256); <====
300 ctx->attr_position = 0;
369 ctx->attr_found = 0;
370 ctx->attr_position = 0;
371 memset(ctx->attr, '\0', 256); <====
407 char
408 slapi_filter_sprintf(const char fmt, ...)
409 {
410 struct filter_ctx ctx;
411 va_list args;
412 char buf;
413 int rc;
414
415 buf = slapi_ch_calloc(sizeof(char), 128 + 1);
416 ctx.buf = buf;
417 memset(ctx.attr,'\0', 256); <====
418 ctx.attr_position = 0;
419 ctx.attr_found = 0;
420 ctx.buf_len = 128;
}}}

358 extra_space = (ctx->buf_len + filter_len + 16);
And may I ask why '16'?

No real reason, just a number that's not too large and not too small. I figured most values would not be over 128 chars, and if they were, they probably weren't much longer.

New patch being attached...

Looks good. Thanks, Mark!

attachment
0001-Ticket-328-make-sure-all-internal-search-filters-are.patch

[mareynol@localhost servers]$ git merge ticket328
Updating 950c0c6..3cf9a52
Fast-forward
ldap/servers/plugins/acl/acllas.c | 17 +-
ldap/servers/plugins/memberof/memberof.c | 24 ++-
ldap/servers/plugins/referint/referint.c | 5 +-
ldap/servers/plugins/replication/urp.c | 3 +-
.../plugins/replication/windows_protocol_util.c | 90 +++----
ldap/servers/slapd/attr.c | 60 ++--
ldap/servers/slapd/filter.c | 51 ++--
ldap/servers/slapd/ldaputil.c | 145 ++++++-----
ldap/servers/slapd/libslapd.def | 4 +-
ldap/servers/slapd/main.c | 8 +-
ldap/servers/slapd/mapping_tree.c | 14 +-
ldap/servers/slapd/plugin.c | 2 +-
ldap/servers/slapd/slapi-plugin.h | 15 +
ldap/servers/slapd/slapi-private.h | 1 -
ldap/servers/slapd/util.c | 278 +++++++++++++++++++-
15 files changed, 501 insertions(+), 216 deletions(-)

[mareynol@localhost servers]$ git push origin master
Counting objects: 49, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (25/25), done.
Writing objects: 100% (25/25), 7.58 KiB, done.
Total 25 (delta 21), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
950c0c6..3cf9a52 master -> master

Added initial screened field value.

move closed tickets to 1.3.0.a1

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/328

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.