| Message ID | 20231022155101.427893-1-sander@svanheule.net |
|---|---|
| State | Under Review |
| Delegated to: | Sander Vanheule |
| Headers | show |
| Series | [libubox] jshn: Do not perform double key normalisation | expand |
Cc Felix's correct address. Sorry for the noise! Best, Sander On Sun, 2023-10-22 at 17:51 +0200, Sander Vanheule wrote: > jshn stores a JSON file in the shell environment using a set of > variable, using the key to an object in the shell variable name. This > restricts the allowed character set to numbers, letters, and underscore. > > Commit a1a97eb11e89 ("jshn: support using characters in elements that do > not conform to shell variable restrictions") added normalisation to > jshn.sh, to ensure the shell functions produce legal variable names. The > original key name is stored in a 'N_normalised_key' variable, to enable > 'jshn -o file.json' to create a JSON file with the original key name. > > Earlier code from commit fda6079b30a4 ("add jshn") also included the > necessary code to perform character set normalisation while building > variable names, which is triggered when loading files with 'jshn -R > file.json'. While add_json_element() will normalise key names, it will > not produce the key names lookup variables, as this concept had not been > introduced yet. > > The former change resulted in double name normalisation being performed > when loading an existing file into the environment, causing the original > key name to be replaced by its normalised version: > > # cat board.json > { > "network-device": { > "foo": "bar" > } > } > > # /usr/bin/jshn -R board.json > json_init; > json_add_object 'network_device'; > json_add_string 'foo' 'bar'; > json_close_object; > > By removing key normalisation from jshn.c, _json_add_generic() now has > the required information again to correctly store the original key, > ensuring repeated operations of 'jshn -R' and 'jshn -o' do not modify > the original JSON file (modulo formatting). > > Fixes: a1a97eb11e89 ("jshn: support using characters in elements that do not conform to > shell variable restrictions") > Signed-off-by: Sander Vanheule <sander@svanheule.net> > --- > This issue was uncovered by the people debugging a recent issue on > Realtek switches [1]. A JSON section was used with the name > 'network-device'. As per the above description, this would be normalised > to 'network_device'. The key name was correctly stored, as long as the > file adding that section (02_network) was the last one to be run. > > OpenWrt commit 4ebba8a05d09 ("realtek: add support for HPE > 1920-8g-poe+") had added a file 03_gpio running after 02_network, > resulting a call to 'jshn -R' and silent normalisation of > 'network-device' to 'network_device'. > > The issue was fixed in OpenWrt and netifd by just always using the > normalised key version. Note that even with this change applied, there > is still no way with jshn to represent keys with the same normalisation > (e.g. 'foo-bar' and 'foo_bar') in one file. Fixing that would require > properly escaping key names in some way when building variable names. > > [1] https://forum.openwrt.org/t/57875/2589 > > Cc: Christian Marangi <ansuelsmth@gmail.com> > Cc: Felix Fietkau <nbd@openwrt.org> > Cc: Jan Hoffmann <jan@3e8.eu> > Cc: Michael Weinrich <michael@a5ap.net> > > > jshn.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/jshn.c b/jshn.c > index 1b685e5fb0d8..97873a220d64 100644 > --- a/jshn.c > +++ b/jshn.c > @@ -96,14 +96,6 @@ static void add_json_string(const char *str) > fwrite(ptr, len, 1, stdout); > } > > -static void write_key_string(const char *key) > -{ > - while (*key) { > - putc(isalnum(*key) ? *key : '_', stdout); > - key++; > - } > -} > - > static int add_json_element(const char *key, json_object *obj) > { > char *type; > @@ -135,7 +127,7 @@ static int add_json_element(const char *key, json_object *obj) > } > > fprintf(stdout, "json_add_%s '", type); > - write_key_string(key); > + add_json_string(key); > > switch (json_object_get_type(obj)) { > case json_type_object:
diff --git a/jshn.c b/jshn.c index 1b685e5fb0d8..97873a220d64 100644 --- a/jshn.c +++ b/jshn.c @@ -96,14 +96,6 @@ static void add_json_string(const char *str) fwrite(ptr, len, 1, stdout); } -static void write_key_string(const char *key) -{ - while (*key) { - putc(isalnum(*key) ? *key : '_', stdout); - key++; - } -} - static int add_json_element(const char *key, json_object *obj) { char *type; @@ -135,7 +127,7 @@ static int add_json_element(const char *key, json_object *obj) } fprintf(stdout, "json_add_%s '", type); - write_key_string(key); + add_json_string(key); switch (json_object_get_type(obj)) { case json_type_object:
jshn stores a JSON file in the shell environment using a set of variable, using the key to an object in the shell variable name. This restricts the allowed character set to numbers, letters, and underscore. Commit a1a97eb11e89 ("jshn: support using characters in elements that do not conform to shell variable restrictions") added normalisation to jshn.sh, to ensure the shell functions produce legal variable names. The original key name is stored in a 'N_normalised_key' variable, to enable 'jshn -o file.json' to create a JSON file with the original key name. Earlier code from commit fda6079b30a4 ("add jshn") also included the necessary code to perform character set normalisation while building variable names, which is triggered when loading files with 'jshn -R file.json'. While add_json_element() will normalise key names, it will not produce the key names lookup variables, as this concept had not been introduced yet. The former change resulted in double name normalisation being performed when loading an existing file into the environment, causing the original key name to be replaced by its normalised version: # cat board.json { "network-device": { "foo": "bar" } } # /usr/bin/jshn -R board.json json_init; json_add_object 'network_device'; json_add_string 'foo' 'bar'; json_close_object; By removing key normalisation from jshn.c, _json_add_generic() now has the required information again to correctly store the original key, ensuring repeated operations of 'jshn -R' and 'jshn -o' do not modify the original JSON file (modulo formatting). Fixes: a1a97eb11e89 ("jshn: support using characters in elements that do not conform to shell variable restrictions") Signed-off-by: Sander Vanheule <sander@svanheule.net> --- This issue was uncovered by the people debugging a recent issue on Realtek switches [1]. A JSON section was used with the name 'network-device'. As per the above description, this would be normalised to 'network_device'. The key name was correctly stored, as long as the file adding that section (02_network) was the last one to be run. OpenWrt commit 4ebba8a05d09 ("realtek: add support for HPE 1920-8g-poe+") had added a file 03_gpio running after 02_network, resulting a call to 'jshn -R' and silent normalisation of 'network-device' to 'network_device'. The issue was fixed in OpenWrt and netifd by just always using the normalised key version. Note that even with this change applied, there is still no way with jshn to represent keys with the same normalisation (e.g. 'foo-bar' and 'foo_bar') in one file. Fixing that would require properly escaping key names in some way when building variable names. [1] https://forum.openwrt.org/t/57875/2589 Cc: Christian Marangi <ansuelsmth@gmail.com> Cc: Felix Fietkau <nbd@openwrt.org> Cc: Jan Hoffmann <jan@3e8.eu> Cc: Michael Weinrich <michael@a5ap.net> jshn.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)