Add valid_encoding checking before inserting into xml object. #318

hsong-rh · 2017-12-01T15:21:53Z

Some special control characters, here '\u000F' and '\u001F', are valid chars in UTF-8 encoding, but unacceptable when we try to convert them into XML. A check is added to remove them first before xml conversion.

https://bugzilla.redhat.com/show_bug.cgi?id=1518249

hsong-rh · 2017-12-01T15:35:03Z

@roliveri @jerryk55 Please review.

roliveri · 2017-12-01T18:08:47Z

lib/gems/pending/util/xml/miq_rexml.rb

@@ -148,7 +148,7 @@ def initialize(first, second = nil, parent = nil)
        rescue => err
          if err.class == ::Encoding::CompatibilityError
            second_utf8 = second.to_s.force_encoding('UTF-8')


Shouldn't this just be changed to something like this:

second_utf8 = second.to_s.encode('UTF-8', :invalid => :replace, :undef => :replace, :replace => "")

The string may not be UTF-8, so you don't want to force the encoding. You want to transcode the string to UTF-8, removing the characters that can't be transcoded.

I tested the above replacement. It didn't work for our case. The final result still has unacceptable chars. The test string I used is the 1st partition type: \xAF=\xC6\x0F\x83\x84rG\x8Ey=i\xD8G}\xE4.

irb(main)> "\xAF=\xC6\x0F\x83\x84rG\x8Ey=i\xD8G}\xE4".encode('UTF-8', :invalid => :replace, :undef => :replace, :replace => "")
=> "=\u000FrGy=iG}"

What makes you say it contains unacceptable characters? If you transcoded the string as I suggested, the result has to be a valid UTF-8 string. Did vaild_encoding? on the resultant string return false?

The valid_encoding? return true. But somehow we still got error message: incompatible encoding regexp match (UTF-8 regexp with ASCII-8BIT string) in evm.log

roliveri · 2017-12-01T18:09:17Z

lib/gems/pending/util/xml/xml_utils.rb

+  # Validate attributes before inserting into xml
+  def self.validate_attrs(h)
+    return nil if h.nil?
+    h.inject({}) { |options, (key, value)| options[key.to_s] = value.to_s.force_encoding('UTF-8').valid_encoding? ? value : "Invalid encoding found"; options }


Same here as above.

Same reason above.

blomquisg · 2017-12-07T18:43:36Z

@roliveri and @hsong-rh, this is for a G-release blocker bug. Try to resolve the discussion quickly so we can get this merged and move the BZ off the blocker list.

roliveri · 2017-12-07T19:02:31Z

@blomquisg What's the reasoning behind making this a blocker?
As it only happens on a subset of VMs.

hsong-rh · 2017-12-07T20:28:34Z

@roliveri I made changes based on your comments. Please review.

roliveri · 2017-12-07T20:43:08Z

lib/gems/pending/util/xml/xml_utils.rb

@@ -62,6 +62,30 @@ def self.findElementInt(paths, ele)
 end

 class XmlHelpers
+  # reference from REXML::TEXT
+  VALID_CHAR = [


What do these characters represent? Are they UTF-8 characters?
Why do we need to do this? Are there valid UTF-8 characters that are not valid in attributes?

Why are you making a copy of something that already exists? Prefer VALID_CHAR = REXML::Text::VALID_CHAR (Or even better just don't create an alias for it at all).

@roliveri After encoding with replacement, they are UTF-8 characters. But some inside chars are still marked as invalid in REXML::Text::check method call, when try to add them as element's attribute.

@Fryguy I'll check to see if any existing codes can help us to do the same thing.

Fryguy

I have a strong feeling that there exists code already in REXML or Nokogiri that already does this and we should not reinvent the wheel.

Since there are no tests, though, I have no idea what this is trying to solve (Additional description in the PR body aside from a BZ link would be helpful too) . Please add tests at least for the XmlHelpers changes.

hsong-rh · 2017-12-11T14:37:19Z

Both REXML and Nokegiri will raise exception in this case. They don't expect such control characters in XML data. I'll add a spec test for this change.

miq-bot · 2017-12-11T17:04:13Z

Checked commits hsong-rh/manageiq-gems-pending@e612f39~...2d76f13 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 3 offenses detected

lib/gems/pending/util/xml/xml_utils.rb

⚠️ - Line 68, Col 7 - Lint/EmptyWhen - Avoid when branches without a body.
⚠️ - Line 78, Col 39 - Lint/ShadowingOuterLocalVariable - Shadowing outer local variable - h.
❗ - Line 88, Col 1 - Style/EmptyLinesAroundClassBody - Extra empty line detected at class body beginning.

hsong-rh · 2017-12-11T21:18:43Z

@roliveri @Fryguy Please review.

simaishi · 2017-12-19T19:04:33Z

gaprindashvili/yes ?

roliveri · 2017-12-19T19:16:19Z

@simaishi Sorry, Yes.

Add valid_encoding checking before inserting into xml object. (cherry picked from commit e04c65f) https://bugzilla.redhat.com/show_bug.cgi?id=1530726

simaishi · 2018-01-03T16:55:00Z

Gaprindashvili backport details:

$ git log -1
commit d865ee63c000e355bb07ed2c4774be2ebe4cbd5b
Author: Richard Oliveri <[email protected]>
Date:   Fri Dec 15 10:21:24 2017 -0500

    Merge pull request #318 from hsong-rh/xml_encoding
    
    Add valid_encoding checking before inserting into xml object.
    (cherry picked from commit e04c65fce73dbeced56f66d724cdb79c69707573)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1530726

Add valid_encoding checking before insert into xml object.

e612f39

https://bugzilla.redhat.com/show_bug.cgi?id=1518249

roliveri suggested changes Dec 1, 2017

View reviewed changes

blomquisg added blocker bug labels Dec 7, 2017

add filter to remove special chars

ed7cb35

roliveri reviewed Dec 7, 2017

View reviewed changes

Fryguy requested changes Dec 7, 2017

View reviewed changes

Add spec test

2d76f13

roliveri approved these changes Dec 15, 2017

View reviewed changes

roliveri merged commit e04c65f into ManageIQ:master Dec 15, 2017

roliveri added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 15, 2017

roliveri added the gaprindashvili/yes label Dec 19, 2017

simaishi pushed a commit that referenced this pull request Jan 3, 2018

Merge pull request #318 from hsong-rh/xml_encoding

d865ee6

Add valid_encoding checking before inserting into xml object. (cherry picked from commit e04c65f) https://bugzilla.redhat.com/show_bug.cgi?id=1530726

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add valid_encoding checking before inserting into xml object. #318

Add valid_encoding checking before inserting into xml object. #318

hsong-rh commented Dec 1, 2017 •

edited

Loading

hsong-rh commented Dec 1, 2017

roliveri Dec 1, 2017

hsong-rh Dec 1, 2017

roliveri Dec 1, 2017

hsong-rh Dec 4, 2017

roliveri Dec 1, 2017

hsong-rh Dec 1, 2017

blomquisg commented Dec 7, 2017

roliveri commented Dec 7, 2017

hsong-rh commented Dec 7, 2017

roliveri Dec 7, 2017

Fryguy Dec 7, 2017 •

edited

Loading

hsong-rh Dec 7, 2017 •

edited

Loading

hsong-rh Dec 7, 2017

Fryguy left a comment

hsong-rh commented Dec 11, 2017

miq-bot commented Dec 11, 2017

hsong-rh commented Dec 11, 2017

simaishi commented Dec 19, 2017

roliveri commented Dec 19, 2017

simaishi commented Jan 3, 2018

Add valid_encoding checking before inserting into xml object. #318

Add valid_encoding checking before inserting into xml object. #318

Conversation

hsong-rh commented Dec 1, 2017 • edited Loading

hsong-rh commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blomquisg commented Dec 7, 2017

roliveri commented Dec 7, 2017

hsong-rh commented Dec 7, 2017

Choose a reason for hiding this comment

Fryguy Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

hsong-rh Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy left a comment

Choose a reason for hiding this comment

hsong-rh commented Dec 11, 2017

miq-bot commented Dec 11, 2017

hsong-rh commented Dec 11, 2017

simaishi commented Dec 19, 2017

roliveri commented Dec 19, 2017

simaishi commented Jan 3, 2018

hsong-rh commented Dec 1, 2017 •

edited

Loading

Fryguy Dec 7, 2017 •

edited

Loading

hsong-rh Dec 7, 2017 •

edited

Loading