From 7818f6e4945e0e3324e6d3f9a7cd444e953d96d8 Mon Sep 17 00:00:00 2001 From: Jonah Graham Date: Fri, 14 Feb 2020 15:30:41 -0500 Subject: [PATCH] Bug 558809: Handle cases where Oomph corrupts \0 char in preference Some CDT preferences use \0 as a separator in preferences. Somewhere in the Oomph preference synchronizer stack there is, or was, a place that failed to escape/unescape preferences with encoded \0 properly. CDT would then fail to parse the preference and an exception would be raised, causing code completions and the editor to be broken. This patch hardens the CDT code to: (1) Allow an escaped \0 to be used as a separator on read (Oomph uses ${0x0}) (2) Handle NumberFormatExceptions gracefully. In this case that means showing user a pop-up that their completion preferences are empty and offering to reset them, or edit them in preference page. This UI logic already existed, so all the new code has to do on failed parse is return a list of all disabled completions. Change-Id: Ibf3b05c0855bb96c195ca43139a50c27a2a90c7e --- ...nProposalComputerPreferenceParserTest.java | 81 +++++++++++++++++++ .../contentassist/ContentAssistTestSuite.java | 2 + .../CodeAssistAdvancedConfigurationBlock.java | 28 ++++--- ...etionProposalComputerPreferenceParser.java | 78 ++++++++++++++++++ .../CompletionProposalComputerRegistry.java | 49 ++++++----- 5 files changed, 206 insertions(+), 32 deletions(-) create mode 100644 core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/CompletionProposalComputerPreferenceParserTest.java create mode 100644 core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerPreferenceParser.java diff --git a/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/CompletionProposalComputerPreferenceParserTest.java b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/CompletionProposalComputerPreferenceParserTest.java new file mode 100644 index 00000000000..7afe5feff54 --- /dev/null +++ b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/CompletionProposalComputerPreferenceParserTest.java @@ -0,0 +1,81 @@ +/******************************************************************************* + * Copyright (c) 2020 Kichwa Coders Canada Inc. and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.eclipse.cdt.ui.tests.text.contentassist; + +import static java.util.Arrays.asList; +import static org.eclipse.cdt.internal.ui.text.contentassist.CompletionProposalComputerPreferenceParser.parseCategoryOrder; +import static org.eclipse.cdt.internal.ui.text.contentassist.CompletionProposalComputerPreferenceParser.parseExcludedCategories; +import static org.junit.Assert.assertEquals; + +import java.text.ParseException; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import org.junit.Test; + +public class CompletionProposalComputerPreferenceParserTest { + + @Test + public void testParseExcludedCategories() throws ParseException { + assertEquals(asSet(), parseExcludedCategories("\0")); + assertEquals(asSet(), parseExcludedCategories("\0\0")); + assertEquals(asSet("cat1"), parseExcludedCategories("cat1\0")); + assertEquals(asSet("cat1", "cat2"), parseExcludedCategories("cat1\0cat2")); + assertEquals(asSet("cat1"), parseExcludedCategories("cat1${0x0}")); + assertEquals(asSet("cat1", "cat2"), parseExcludedCategories("cat1${0x0}cat2")); + assertEquals(asSet("cat1"), parseExcludedCategories("cat1$$$${0x0}")); + assertEquals(asSet("cat1", "cat2"), parseExcludedCategories("cat1$$$${0x0}cat2")); + assertEquals(asSet("cat1", "cat2", "cat3", "cat4"), + parseExcludedCategories("cat1$$$${0x0}cat2${0x0}cat3\0cat4\0")); + } + + @Test + public void testParseCategoryOrder() throws ParseException { + assertEquals(asMap("cat1", 1), parseCategoryOrder("cat1:1\0")); + assertEquals(asMap("cat1", 1000), parseCategoryOrder("cat1:1000\0\0")); + assertEquals(asMap("cat1", 1000, "cat2", 2000), parseCategoryOrder("cat1:1000\0cat2:2000")); + assertEquals(asMap("cat1", 1), parseCategoryOrder("cat1:1${0x0}")); + assertEquals(asMap("cat1", 1000, "cat2", 2000), parseCategoryOrder("cat1:1000${0x0}cat2:2000")); + assertEquals(asMap("cat1", 1), parseCategoryOrder("cat1:1$$$$${0x0}")); + assertEquals(asMap("cat1", 1000, "cat2", 2000), parseCategoryOrder("cat1:1000$$$$${0x0}cat2:2000")); + assertEquals(asMap("cat1", 1000, "cat2", 2000, "cat3", 3000, "cat4", 4000), + parseCategoryOrder("cat1:1000$$$$${0x0}cat2:2000${0x0}cat3:3000\0cat4:4000\0")); + } + + @Test(expected = ParseException.class) + public void testParseIntFailsGracefully1() throws ParseException { + assertEquals(asMap(), parseCategoryOrder("cat1:this is not a number\0")); + } + + @Test(expected = ParseException.class) + public void testParseIntFailsGracefully2() throws ParseException { + assertEquals(asMap(), parseCategoryOrder("cat1 missing number\0")); + } + + @Test(expected = ParseException.class) + public void testParseIntFailsGracefully3() throws ParseException { + assertEquals(asMap(), parseCategoryOrder("cat1:0:extra field\0")); + } + + private Set asSet(String... elem) { + return new HashSet<>(asList(elem)); + } + + private Map asMap(Object... elem) { + HashMap map = new HashMap<>(); + for (int i = 0; i < elem.length; i += 2) { + map.put((String) elem[i + 0], (Integer) elem[i + 1]); + } + return map; + } +} diff --git a/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/ContentAssistTestSuite.java b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/ContentAssistTestSuite.java index c95b6f6fb69..290d67c52a4 100644 --- a/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/ContentAssistTestSuite.java +++ b/core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/text/contentassist/ContentAssistTestSuite.java @@ -22,6 +22,8 @@ import org.junit.runners.Suite; ContentAssistTests.class, + CompletionProposalComputerPreferenceParserTest.class, + }) public class ContentAssistTestSuite { } diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/CodeAssistAdvancedConfigurationBlock.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/CodeAssistAdvancedConfigurationBlock.java index 86927a8c57f..2f286342a2f 100644 --- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/CodeAssistAdvancedConfigurationBlock.java +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/preferences/CodeAssistAdvancedConfigurationBlock.java @@ -14,6 +14,7 @@ *******************************************************************************/ package org.eclipse.cdt.internal.ui.preferences; +import java.text.ParseException; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; @@ -21,10 +22,12 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import org.eclipse.cdt.internal.ui.dialogs.IStatusChangeListener; import org.eclipse.cdt.internal.ui.dialogs.StatusInfo; import org.eclipse.cdt.internal.ui.text.contentassist.CompletionProposalCategory; +import org.eclipse.cdt.internal.ui.text.contentassist.CompletionProposalComputerPreferenceParser; import org.eclipse.cdt.internal.ui.text.contentassist.CompletionProposalComputerRegistry; import org.eclipse.cdt.internal.ui.util.Messages; import org.eclipse.cdt.internal.ui.util.SWTUtil; @@ -230,22 +233,25 @@ final class CodeAssistAdvancedConfigurationBlock extends OptionsConfigurationBlo } private boolean readInclusionPreference(CompletionProposalCategory cat) { - String[] ids = getTokens(getValue(PREF_EXCLUDED_CATEGORIES), SEPARATOR); - for (String id : ids) { - if (id.equals(cat.getId())) - return false; + String value = getValue(PREF_EXCLUDED_CATEGORIES); + try { + Set parseExcludedCategories = CompletionProposalComputerPreferenceParser + .parseExcludedCategories(value); + return !parseExcludedCategories.contains(cat.getId()); + } catch (ParseException e) { + return true; } - return true; } private int readOrderPreference(CompletionProposalCategory cat) { - String[] sortOrderIds = getTokens(getValue(PREF_CATEGORY_ORDER), SEPARATOR); - for (String sortOrderId : sortOrderIds) { - String[] idAndRank = getTokens(sortOrderId, COLON); - if (idAndRank[0].equals(cat.getId())) - return Integer.parseInt(idAndRank[1]); + String categoryOrderPref = getValue(PREF_CATEGORY_ORDER); + try { + Map parseCategoryOrder = CompletionProposalComputerPreferenceParser + .parseCategoryOrder(categoryOrderPref); + return parseCategoryOrder.getOrDefault(cat.getId(), LIMIT + 1); + } catch (ParseException e) { + return LIMIT + 1; } - return LIMIT + 1; } public void update() { diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerPreferenceParser.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerPreferenceParser.java new file mode 100644 index 00000000000..428baf98a6c --- /dev/null +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerPreferenceParser.java @@ -0,0 +1,78 @@ +/******************************************************************************* + * Copyright (c) 2020 Kichwa Coders Canada Inc. and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.eclipse.cdt.internal.ui.text.contentassist; + +import java.text.ParseException; +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import org.eclipse.cdt.ui.PreferenceConstants; + +/** + * Parses the Completion Proposal Computer Preferences. + *

+ * See org.eclipse.cdt.internal.ui.preferences.CodeAssistAdvancedConfigurationBlock.PreferenceModel + * for the write side of the preferences. + */ +public class CompletionProposalComputerPreferenceParser { + /** + * Parses the {@link PreferenceConstants#CODEASSIST_EXCLUDED_CATEGORIES} value and + * converts to a set of categories that are excluded. + * @param preferenceValue as stored in {@link PreferenceConstants#CODEASSIST_EXCLUDED_CATEGORIES} + * @return set of excluded categories + * @throws ParseException if the value cannot be parsed + */ + public static Set parseExcludedCategories(String preferenceValue) throws ParseException { + Set disabled = new HashSet<>(); + String[] disabledArray = splitOnNulls(preferenceValue); + disabled.addAll(Arrays.asList(disabledArray)); + return disabled; + } + + /** + * Parses the {@link PreferenceConstants#CODEASSIST_CATEGORY_ORDER} value and + * converts to a map of category ids to sort rank number + * @param preferenceValue as stored in {@link PreferenceConstants#CODEASSIST_CATEGORY_ORDER} + * @return map of category id to rank order + * @throws ParseException if the value cannot be parsed + */ + public static Map parseCategoryOrder(String preferenceValue) throws ParseException { + Map ordered = new HashMap<>(); + String[] orderedArray = splitOnNulls(preferenceValue); + for (String entry : orderedArray) { + String[] idRank = entry.split(":"); //$NON-NLS-1$ + if (idRank.length != 2) { + throw new ParseException(entry, 0); + } + String id = idRank[0]; + int rank; + try { + rank = Integer.parseInt(idRank[1]); + } catch (NumberFormatException e) { + throw new ParseException(entry, 0); + } + ordered.put(id, Integer.valueOf(rank)); + } + return ordered; + } + + /** + * See Bug 558809. Oomph seems to have failed at times to encode/decode nul character '\0' + * from the format it is stored in Oomph setup files. We can have ${0x0} instead of \0, infact + * there can be multiple $, so $$$$${0x0} is a valid split too. + */ + private static String[] splitOnNulls(String preference) { + return preference.split("\\000|(\\$+\\{0x0\\})"); //$NON-NLS-1$ + } +} diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerRegistry.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerRegistry.java index 12c66d662e1..33843a56842 100644 --- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerRegistry.java +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/CompletionProposalComputerRegistry.java @@ -14,6 +14,7 @@ *******************************************************************************/ package org.eclipse.cdt.internal.ui.text.contentassist; +import java.text.ParseException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -24,7 +25,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.StringTokenizer; import org.eclipse.cdt.internal.ui.util.Messages; import org.eclipse.cdt.ui.CUIPlugin; @@ -272,19 +272,19 @@ public final class CompletionProposalComputerRegistry { private List getCategories(List elements) { IPreferenceStore store = CUIPlugin.getDefault().getPreferenceStore(); - String preference = store.getString(PreferenceConstants.CODEASSIST_EXCLUDED_CATEGORIES); - Set disabled = new HashSet<>(); - StringTokenizer tok = new StringTokenizer(preference, "\0"); //$NON-NLS-1$ - while (tok.hasMoreTokens()) - disabled.add(tok.nextToken()); - Map ordered = new HashMap<>(); - preference = store.getString(PreferenceConstants.CODEASSIST_CATEGORY_ORDER); - tok = new StringTokenizer(preference, "\0"); //$NON-NLS-1$ - while (tok.hasMoreTokens()) { - StringTokenizer inner = new StringTokenizer(tok.nextToken(), ":"); //$NON-NLS-1$ - String id = inner.nextToken(); - int rank = Integer.parseInt(inner.nextToken()); - ordered.put(id, Integer.valueOf(rank)); + Set disabled = Collections.emptySet(); + Map ordered = Collections.emptyMap(); + boolean parseFailed = false; + try { + disabled = CompletionProposalComputerPreferenceParser + .parseExcludedCategories(store.getString(PreferenceConstants.CODEASSIST_EXCLUDED_CATEGORIES)); + ordered = CompletionProposalComputerPreferenceParser + .parseCategoryOrder(store.getString(PreferenceConstants.CODEASSIST_CATEGORY_ORDER)); + } catch (ParseException e) { + // Failed to parse user setting, clear all settings + // and display error message to user allowing them to + // reset on first use + parseFailed = true; } List categories = new ArrayList<>(); @@ -296,13 +296,20 @@ public final class CompletionProposalComputerRegistry { CompletionProposalCategory category = new CompletionProposalCategory(element, this); categories.add(category); - category.setIncluded(!disabled.contains(category.getId())); - Integer rank = ordered.get(category.getId()); - if (rank != null) { - int r = rank.intValue(); - boolean separate = r < 0xffff; - category.setSeparateCommand(separate); - category.setSortOrder(r); + if (parseFailed) { + // When parse has failed we do the same thing as if the user had disabled + // off ever proposal category. This causes a pop-up on first completion + // attempt with the option of resetting defaults + category.setIncluded(false); + } else { + category.setIncluded(!disabled.contains(category.getId())); + Integer rank = ordered.get(category.getId()); + if (rank != null) { + int r = rank.intValue(); + boolean separate = r < 0xffff; + category.setSeparateCommand(separate); + category.setSortOrder(r); + } } } } catch (CoreException x) {