Skip to content

Commit d8b5e67

Browse files
tsteven4tsteven4
authored
fix mkshort unique (#1168)
* fix mkshort unique. 1. we were off by one when deciding if we could just concatentate the generated suffix to the name, failing to use the last available character. 2. when attempting to copy the generated suffix to a presumably shortened name, and the generated suffix was longer than the name, the target of the copy was outside the name buffer. In the common case that the rank of size_t was greater than the rank of int, the target was well beyond the end of name, not before as one would expect with signed arithmetic. With the new algorithm when the target length is insufficient to fit both the name and the suffix we will only truncate the name as required to fit the truncated name and the complete suffix in the target length(as opposed to the original length of the name). We fatal if this is not possible. Add a test case to exercise the make unique code. * fix reference mode. * add notes for future enhancements * fix testcase cut and paste booboo * refactor mkshort input from char* to QByteArray.
1 parent 8b3e374 commit d8b5e67

File tree

10 files changed

+666
-27
lines changed

10 files changed

+666
-27
lines changed

‎defs.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -847,7 +847,7 @@ using ff_readposn = Waypoint* (*)(posn_status*);
847847
struct mkshort_handle_imp; // forward declare, definition in mkshort.cc
848848
using short_handle = mkshort_handle_imp*;
849849

850-
QByteArray mkshort(short_handle, const char*, bool);
850+
QByteArray mkshort(short_handle, const QByteArray&, bool);
851851
QString mkshort(short_handle, const QString&);
852852
short_handle mkshort_new_handle();
853853
QString mkshort_from_wpt(short_handle h, const Waypoint* wpt);

‎garmin.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -890,8 +890,8 @@ waypoint_prepare()
890890
*/
891891
QByteArray ident = mkshort(mkshort_handle,
892892
global_opts.synthesize_shortnames ?
893-
str_from_unicode(src).constData() :
894-
str_from_unicode(wpt->shortname).constData(),
893+
str_from_unicode(src) :
894+
str_from_unicode(wpt->shortname),
895895
false);
896896
/* Should not be a strcpy as 'ident' isn't really a C string,
897897
* but rather a garmin "fixed length" buffer that's padded

‎mkshort.cc‎

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,9 @@
1919
2020
*/
2121

22+
#include <cassert> // for assert
2223
#include <cctype> // for isspace, toupper, isdigit
23-
#include <cstdio> // for snprintf, size_t
24+
#include <climits> // for INT_MAX
2425
#include <cstring> // for strlen, memmove, strchr, strcpy, strncmp, strcat, strncpy
2526

2627
#include <QByteArray> // for QByteArray
@@ -49,7 +50,7 @@ class ShortNameKey;
4950
using ShortNameHash = QHash<ShortNameKey, uniq_shortname*>;
5051
class ShortNameKey {
5152
public:
52-
ShortNameKey(char* name) : shortname(name) {} /* converting constructor */
53+
ShortNameKey(const QByteArray& name) : shortname(name) {} /* converting constructor */
5354

5455
friend qhash_result_t qHash(const ShortNameKey &key, qhash_result_t seed = 0) noexcept
5556
{
@@ -109,7 +110,7 @@ mkshort_new_handle()
109110

110111
static
111112
uniq_shortname*
112-
is_unique(mkshort_handle_imp* h, char* name)
113+
is_unique(mkshort_handle_imp* h, const QByteArray& name)
113114
{
114115
if (h->namelist.contains(name)) {
115116
return h->namelist.value(name);
@@ -119,34 +120,38 @@ is_unique(mkshort_handle_imp* h, char* name)
119120

120121
static
121122
void
122-
add_to_hashlist(mkshort_handle_imp* h, char* name)
123+
add_to_hashlist(mkshort_handle_imp* h, const QByteArray& name)
123124
{
124125
h->namelist.insert(name, new uniq_shortname);
125126
}
126127

127-
char*
128-
mkshort_add_to_list(mkshort_handle_imp* h, char* name)
128+
static
129+
void
130+
mkshort_add_to_list(mkshort_handle_imp* h, QByteArray& name)
129131
{
130-
uniq_shortname* s;
132+
static_assert(!std::is_signed<decltype(h->target_len)>::value,
133+
"simplify the following logic if target length is signed.");
134+
assert(h->target_len <= INT_MAX);
135+
int target_len = h->target_len;
131136

137+
uniq_shortname* s;
132138
while ((s = is_unique(h, name))) {
133-
char tbuf[13];
134-
size_t l = strlen(name);
135139

136-
s->conflictctr++;
140+
QByteArray suffix(".");
141+
suffix.append(QByteArray::number(++s->conflictctr));
137142

138-
int dl = snprintf(tbuf, sizeof(tbuf), ".%d", s->conflictctr);
139-
140-
if (l + dl < h->target_len) {
141-
name = (char*) xrealloc(name, l + dl + 1);
142-
strcat(name, tbuf);
143+
if (name.size() + suffix.size() <= target_len) {
144+
name.append(suffix);
145+
} else if (suffix.size() <= target_len) {
146+
// FIXME: utf8 => grapheme truncate
147+
name.truncate(target_len - suffix.size());
148+
name.append(suffix);
143149
} else {
144-
strcpy(&name[l-dl], tbuf);
150+
fatal("mkshort failure, the specified short length is insufficient.\n");
145151
}
146152
}
147153

148154
add_to_hashlist(h, name);
149-
return name;
150155
}
151156

152157
void
@@ -249,7 +254,9 @@ void
249254
setshort_length(short_handle h, int l)
250255
{
251256
auto* hdl = (mkshort_handle_imp*) h;
252-
if (l == 0) {
257+
if (l < 0) {
258+
fatal("mkshort: short length must be non-negative.\n");
259+
} else if (l == 0) {
253260
hdl->target_len = default_target_len;
254261
} else {
255262
hdl->target_len = l;
@@ -354,7 +361,7 @@ setshort_mustuniq(short_handle h, int i)
354361
}
355362

356363
QByteArray
357-
mkshort(short_handle h, const char* istring, bool is_utf8)
364+
mkshort(short_handle h, const QByteArray& istring, bool is_utf8)
358365
{
359366
char* ostring;
360367
char* tstring;
@@ -370,7 +377,7 @@ mkshort(short_handle h, const char* istring, bool is_utf8)
370377
result.remove(QChar::ReplacementCharacter);
371378
ostring = xstrdup(result.toUtf8().constData());
372379
} else {
373-
ostring = xstrdup(istring);
380+
ostring = xstrdup(istring.constData());
374381
}
375382

376383
/*
@@ -555,18 +562,18 @@ mkshort(short_handle h, const char* istring, bool is_utf8)
555562
ostring = xstrdup(hdl->defname);
556563
}
557564

558-
if (hdl->must_uniq) {
559-
ostring = mkshort_add_to_list(hdl, ostring);
560-
}
561565
QByteArray rval(ostring);
562566
xfree(ostring);
567+
if (hdl->must_uniq) {
568+
mkshort_add_to_list(hdl, rval);
569+
}
563570
return rval;
564571
}
565572

566573
QString
567574
mkshort(short_handle h, const QString& istring)
568575
{
569-
return mkshort(h, CSTR(istring), true);
576+
return mkshort(h, istring.toUtf8(), true);
570577
}
571578

572579
/*

‎reference/mkshort.csv‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
lat,lon,name
2+
0,0,wp
3+
0,0,wp
4+
0,0,wp
5+
0,0,wp
6+
0,0,wp
7+
0,0,wp
8+
0,0,wp
9+
0,0,wp
10+
0,0,wp
11+
0,0,wp
12+
0,0,wp
13+
0,0,wp
14+
0,0,wp
15+
0,0,wp
16+
0,0,wp
17+
0,0,wp
18+
0,0,wp
19+
0,0,wp
20+
0,0,wp
21+
0,0,wp
22+
0,0,wp
23+
0,0,wp
24+
0,0,wp
25+
0,0,wp
26+
0,0,wp
27+
0,0,wp
28+
0,0,wp
29+
0,0,wp
30+
0,0,wp
31+
0,0,wp
32+
0,0,wp
33+
0,0,wp
34+
0,0,wp
35+
0,0,wp
36+
0,0,wp
37+
0,0,wp
38+
0,0,wp
39+
0,0,wp
40+
0,0,wp
41+
0,0,wp
42+
0,0,wp
43+
0,0,wp
44+
0,0,wp
45+
0,0,wp
46+
0,0,wp
47+
0,0,wp
48+
0,0,wp
49+
0,0,wp
50+
0,0,wp
51+
0,0,wp
52+
0,0,wp
53+
0,0,wp
54+
0,0,wp
55+
0,0,wp
56+
0,0,wp
57+
0,0,wp
58+
0,0,wp
59+
0,0,wp
60+
0,0,wp
61+
0,0,wp
62+
0,0,wp
63+
0,0,wp
64+
0,0,wp
65+
0,0,wp
66+
0,0,wp
67+
0,0,wp
68+
0,0,wp
69+
0,0,wp
70+
0,0,wp
71+
0,0,wp
72+
0,0,wp
73+
0,0,wp
74+
0,0,wp
75+
0,0,wp
76+
0,0,wp
77+
0,0,wp
78+
0,0,wp
79+
0,0,wp
80+
0,0,wp
81+
0,0,wp
82+
0,0,wp
83+
0,0,wp
84+
0,0,wp
85+
0,0,wp
86+
0,0,wp
87+
0,0,wp
88+
0,0,wp
89+
0,0,wp
90+
0,0,wp
91+
0,0,wp
92+
0,0,wp
93+
0,0,wp
94+
0,0,wp
95+
0,0,wp
96+
0,0,wp
97+
0,0,wp
98+
0,0,wp
99+
0,0,wp
100+
0,0,wp
101+
0,0,wp
102+
0,0,wp
103+
0,0,wp
104+
0,0,wp
105+
0,0,wp
106+
0,0,wp
107+
0,0,wp
108+
0,0,wp
109+
0,0,wp
110+
0,0,wp
111+
0,0,wp
112+
0,0,wp
113+
0,0,wp
114+
0,0,wp
115+
0,0,wp
116+
0,0,wp
117+
0,0,wp
118+
0,0,wp
119+
0,0,wp
120+
0,0,wp
121+
0,0,wp
122+
0,0,wp
123+
0,0,wp
124+
0,0,wp
125+
0,0,wp
126+
0,0,wp
127+
0,0,wp
128+
0,0,wp
129+
0,0,wp
130+
0,0,wp
131+
0,0,wp
132+
0,0,wp
133+
0,0,wp
134+
0,0,wp
135+
0,0,wp
136+
0,0,wp
137+
0,0,wp
138+
0,0,wp
139+
0,0,wp
140+
0,0,wp
141+
0,0,wp
142+
0,0,wp
143+
0,0,wp
144+
0,0,wp
145+
0,0,wp
146+
0,0,wp
147+
0,0,wp
148+
0,0,wp
149+
0,0,wp
150+
0,0,wp
151+
0,0,wp

‎reference/mkshort.style‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# gpsbabel XCSV style file
2+
#
3+
DESCRIPTION Comma separated values
4+
SHORTLEN 4
5+
#
6+
# FILE LAYOUT DEFINITIIONS:
7+
#
8+
FIELD_ENCLOSER DOUBLEQUOTE
9+
FIELD_DELIMITER COMMASPACE
10+
RECORD_DELIMITER NEWLINE
11+
BADCHARS COMMA
12+
#
13+
# INDIVIDUAL DATA FIELDS, IN ORDER OF APPEARANCE:
14+
#
15+
IFIELD LAT_DECIMAL, "", "%8.5f"
16+
IFIELD LON_DECIMAL, "", "%8.5f"
17+
IFIELD SHORTNAME, "", "%s"

‎reference/mkshort3.log‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
mkshort failure, the specified short length is insufficient.

0 commit comments

Comments
 (0)