Skip to content

ASCIIPropertyListParser: allow escaping chars that needn't be escaped#45

Merged
3breadt merged 1 commit into
3breadt:masterfrom
matvore:escape
Aug 18, 2018
Merged

ASCIIPropertyListParser: allow escaping chars that needn't be escaped#45
3breadt merged 1 commit into
3breadt:masterfrom
matvore:escape

Conversation

@matvore

@matvore matvore commented Jul 25, 2018

Copy link
Copy Markdown
Contributor

No description provided.

@3breadt

3breadt commented Aug 14, 2018

Copy link
Copy Markdown
Owner

At the moment invalid escape sequences would lead to a failure in parsing the file. This is acceptable I believe.

With your commit they would be treated as if they were correct. Furthermore your code even assumes they have a special meaning, that the following character is just needlessly escaped. But there is no way to be sure that, for example, "\d" should be interpreted as "d" and not as "$". The creator of the property list may have defined his own custom escape sequences, only supported by his own parser. Or he may have simply made a mistake.

Whatever the reason for the invalid escape sequence, it remains invalid. Thus, I won't accept your pull request.

@3breadt 3breadt closed this Aug 14, 2018
@matvore

matvore commented Aug 14, 2018

Copy link
Copy Markdown
Contributor Author

Are you aware that "defaults" interprets the characters as my pull request does (this was in the code, I'm not sure if you caught it):

$ printf '"a" = "\\c";\n' > /tmp/foo.plist
$ defaults read /tmp/foo.plist
{
    a = c;
}

@3breadt

3breadt commented Aug 14, 2018

Copy link
Copy Markdown
Owner

No I was not aware of it. But after fiddling with it a bit, I see it does not really escape all invalid characters consistently. Also XCode's property list viewer handles the invalid escape sequence even worse. I really would not like to take either program's behavior as a pattern for this library.
screenshot

@matvore

matvore commented Aug 14, 2018

Copy link
Copy Markdown
Contributor Author

\a is an alert bell
\f is formfeed
\v is vertical tab

(see https://en.wikipedia.org/wiki/Escape_sequences_in_C)

These characters are apparently not supported in ASCII plists. Remove them from your sample string and you'll get consistent output between defaults and Xcode. So I see two options:

  • keep pull request as-is but issue an error for \a \f or \v
  • change pull request to only work for ' or " and all non-necessary escape characters cause an exception like they used to (supporting quotes was the original intent of the change, but I didn't realize \a \f and \v were exceptional at the time I wrote it)

Which do you prefer? I'm happy with either one.

@3breadt

3breadt commented Aug 14, 2018

Copy link
Copy Markdown
Owner

I already improved the exception text thrown when an invalid escape sequence is encountered in b8d6987.

The only actual reference describing the ASCII property list format I found at http://www.gnustep.org/resources/documentation/Developer/Base/Reference/NSPropertyList.html
If I go by that, single quotes would not be allowed.

In reality tools like defaultsmay implement the complete set of (Objective-)C escape sequences. I would be ok with also adding support for that well-defined set of escape sequences. This would mean adding support for:

  • \a
  • \f
  • \v
  • \'
  • \?
  • \x...

Would that be fine with you?

@3breadt 3breadt reopened this Aug 14, 2018
@matvore

matvore commented Aug 15, 2018

Copy link
Copy Markdown
Contributor Author

Not sure what \? is. I would just throw an exception on \a \f and \v, since they appeared mangled in Xcode and defaults interpreted them as \\a. Other than that, the NSPropertyList.html document and your list are fine.

I'm mostly concerned about the single quote. We had tooling which escaped these single quotes (maybe it was an old version of Xcode, or maybe internal tooling) when it wrote Localizable.strings files - the Xcode build process is fine with these.

@3breadt

3breadt commented Aug 15, 2018

Copy link
Copy Markdown
Owner

Alright, now I've just added a switch case to allow for the single quote (See 44ecdc2). I will leave out the other C escape sequences for the moment as nobody else was bothered by the missing support so far.

@matvore

matvore commented Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

Great! Thank you.

@matvore

matvore commented Aug 17, 2018

Copy link
Copy Markdown
Contributor Author

I changed this PR's commit to simply add a test for escaping the single quote. Can you merge it in the current form?

@3breadt 3breadt merged commit b14e170 into 3breadt:master Aug 18, 2018
@3breadt

3breadt commented Aug 18, 2018

Copy link
Copy Markdown
Owner

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants