You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2024/01/15 10:16:00 UTC
(tomcat) branch main updated: Fix logic bug spotted by Coverity Scan
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new bee714eb1d Fix logic bug spotted by Coverity Scan
bee714eb1d is described below
commit bee714eb1dabfcc07bf410e1145f6c580f14539d
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Jan 15 10:11:59 2024 +0000
Fix logic bug spotted by Coverity Scan
---
.../apache/tomcat/util/digester/ServiceBindingPropertySource.java | 2 +-
webapps/docs/changelog.xml | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java b/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
index 89617c9cfb..bd06630f01 100644
--- a/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
+++ b/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
@@ -116,7 +116,7 @@ public class ServiceBindingPropertySource implements IntrospectionUtils.Property
int length = bytes.length;
if (chomp) {
- if(length > 1 && bytes[length - 2] == '\r' && bytes[length - 2] == '\n') {
+ if(length > 1 && bytes[length - 2] == '\r' && bytes[length - 1] == '\n') {
length -= 2;
} else if (length > 0) {
byte c = bytes[length - 1];
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 6e9ffb917c..aa7fce0034 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -120,6 +120,11 @@
that is no longer included in the JAR. Based on pull request
<pr>684</pr> by Jendrik Johannes. (markt)
</fix>
+ <fix>
+ Fix ServiceBindingPropertySource so that trailing <code>\r\n</code>
+ sequences are correctly removed from files containing property values
+ when configured to do so. Bug identified by Coverity Scan. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Fix logic bug spotted by Coverity Scan
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Konstantin,
On 1/15/24 09:30, Konstantin Kolinko wrote:
> пн, 15 янв. 2024 г. в 13:16, <ma...@apache.org>:
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> markt pushed a commit to branch main
>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>
>>
>> The following commit(s) were added to refs/heads/main by this push:
>> new bee714eb1d Fix logic bug spotted by Coverity Scan
>> bee714eb1d is described below
>>
>> commit bee714eb1dabfcc07bf410e1145f6c580f14539d
>> Author: Mark Thomas <ma...@apache.org>
>> AuthorDate: Mon Jan 15 10:11:59 2024 +0000
>>
>> Fix logic bug spotted by Coverity Scan
>> ---
>> .../apache/tomcat/util/digester/ServiceBindingPropertySource.java | 2 +-
>> webapps/docs/changelog.xml | 5 +++++
>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java b/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
>> index 89617c9cfb..bd06630f01 100644
>> --- a/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
>> +++ b/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
>> @@ -116,7 +116,7 @@ public class ServiceBindingPropertySource implements IntrospectionUtils.Property
>> int length = bytes.length;
>>
>> if (chomp) {
>> - if(length > 1 && bytes[length - 2] == '\r' && bytes[length - 2] == '\n') {
>> + if(length > 1 && bytes[length - 2] == '\r' && bytes[length - 1] == '\n') {
>> length -= 2;
>> } else if (length > 0) {
>> byte c = bytes[length - 1];
>> diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
>> index 6e9ffb917c..aa7fce0034 100644
>> --- a/webapps/docs/changelog.xml
>> +++ b/webapps/docs/changelog.xml
>> @@ -120,6 +120,11 @@
>> that is no longer included in the JAR. Based on pull request
>> <pr>684</pr> by Jendrik Johannes. (markt)
>> </fix>
>> + <fix>
>> + Fix ServiceBindingPropertySource so that trailing <code>\r\n</code>
>> + sequences are correctly removed from files containing property values
>> + when configured to do so. Bug identified by Coverity Scan. (markt)
>> + </fix>
>> </changelog>
>> </subsection>
>> <subsection name="Coyote">
>
> Reviewing the code of ServiceBindingPropertySource,
>
> 1. It removes only the last end-of-line sequence (one or two characters).
> It could be rewritten with a loop to remove any \n \r characters that
> are found at the end.
This was intentional (only trim the final newline) but could be updated
if the team thinks it makes sense to do so.
Maybe text-editing programs append a trailing newline at the end of text
files unless you take special steps to prevent it from happening. It's
usually easy to limit the trailing newlines to a single one.
> 2. The code later uses "return new String(bytes, 0, length);"
> to create a String, without specifying any encoding.
This was intentional as well, as the file is originally read without
specifying the encoding. Allowing an admin to specify the encoding would
have expanded the configuration for ServiceBrindingPropertySource and my
intent at the time was to make the fewest number of changes possible.
> I guess that the Platform default encoding nowadays is UTF-8.
> Or maybe it makes sense to use the default encoding, as people edit
> those files, using whatever default encoding they prefer.
+1
A PropertySource is specified using a system property. Specifying the
encoding for a file-based PropertySource would mean having another
system property or a more complicated configuration scheme for
PropertySource.
I'm happy to entertain ideas for improvement.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Fix logic bug spotted by Coverity Scan
Posted by Konstantin Kolinko <kn...@gmail.com>.
пн, 15 янв. 2024 г. в 13:16, <ma...@apache.org>:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new bee714eb1d Fix logic bug spotted by Coverity Scan
> bee714eb1d is described below
>
> commit bee714eb1dabfcc07bf410e1145f6c580f14539d
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Mon Jan 15 10:11:59 2024 +0000
>
> Fix logic bug spotted by Coverity Scan
> ---
> .../apache/tomcat/util/digester/ServiceBindingPropertySource.java | 2 +-
> webapps/docs/changelog.xml | 5 +++++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java b/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
> index 89617c9cfb..bd06630f01 100644
> --- a/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
> +++ b/java/org/apache/tomcat/util/digester/ServiceBindingPropertySource.java
> @@ -116,7 +116,7 @@ public class ServiceBindingPropertySource implements IntrospectionUtils.Property
> int length = bytes.length;
>
> if (chomp) {
> - if(length > 1 && bytes[length - 2] == '\r' && bytes[length - 2] == '\n') {
> + if(length > 1 && bytes[length - 2] == '\r' && bytes[length - 1] == '\n') {
> length -= 2;
> } else if (length > 0) {
> byte c = bytes[length - 1];
> diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> index 6e9ffb917c..aa7fce0034 100644
> --- a/webapps/docs/changelog.xml
> +++ b/webapps/docs/changelog.xml
> @@ -120,6 +120,11 @@
> that is no longer included in the JAR. Based on pull request
> <pr>684</pr> by Jendrik Johannes. (markt)
> </fix>
> + <fix>
> + Fix ServiceBindingPropertySource so that trailing <code>\r\n</code>
> + sequences are correctly removed from files containing property values
> + when configured to do so. Bug identified by Coverity Scan. (markt)
> + </fix>
> </changelog>
> </subsection>
> <subsection name="Coyote">
Reviewing the code of ServiceBindingPropertySource,
1. It removes only the last end-of-line sequence (one or two characters).
It could be rewritten with a loop to remove any \n \r characters that
are found at the end.
2. The code later uses "return new String(bytes, 0, length);"
to create a String, without specifying any encoding.
I guess that the Platform default encoding nowadays is UTF-8.
Or maybe it makes sense to use the default encoding, as people edit
those files, using whatever default encoding they prefer.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org