Skip to content

Commit a21ef29

Browse files
rijnbclaude
andcommitted
Fix critical and major code review issues
- Replace memcmp-based NaN/Inf detection with isnan()/isinf() from <math.h> (portable, correct for all NaN bit patterns, and now checks latDeg for Inf too) - Fix off-by-one in encodeLatLonToSelectedMapcode: > should be >= for index bound - Remove always-false ASSERT after *s=0 in encodeExtension - Fix convertFromAbjad/convertToAbjad to null-check strchr return before arithmetic - Allow TERRITORY_NONE/TERRITORY_UNKNOWN in encodeLatLonToSingleMapcode per docs - Fix convertUtf16ToUtf8 to return start pointer instead of post-null end pointer - Explicitly initialize GLOBAL_MAKEISO_PTR (was accidentally correct via NULL) - Replace sprintf with snprintf in convertToRoman (mapcode_legacy.c) - Change UWORD from unsigned short int to uint16_t; add #include <stdint.h> - Replace magic constant 128 with MAX_MAPCODE_RESULT_ASCII_LEN in encoderEngine - Add regression tests for all fixed bugs in testBugFixes() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 2634d18 commit a21ef29

5 files changed

Lines changed: 99 additions & 25 deletions

File tree

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,3 +132,4 @@ Release
132132
# -----------------------------------------------------------------------------
133133
nb-configuration.xml
134134
*.orig
135+
.gitnexus

mapcodelib/mapcode_legacy.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
*/
2424
static Mapcodes GLOBAL_RESULT;
2525
static char GLOBAL_MAKEISO_BUFFER[2 * (MAX_ISOCODE_LEN + 1)];
26-
static char* GLOBAL_MAKEISO_PTR;
26+
static char* GLOBAL_MAKEISO_PTR = GLOBAL_MAKEISO_BUFFER + (MAX_ISOCODE_LEN + 1);
2727

2828

2929
int encodeLatLonToMapcodes_Deprecated(
@@ -97,12 +97,12 @@ char* convertToRoman(char* asciiBuffer, int maxLength, const UWORD* unicodeBuffe
9797
}
9898
if (!err) {
9999
char romanized[MAX_MAPCODE_RESULT_LEN];
100-
sprintf(romanized, "%s%s%s%s%s",
101-
mapcodeElements.territoryISO,
102-
*mapcodeElements.territoryISO ? " " : "",
103-
mapcodeElements.properMapcode,
104-
*mapcodeElements.precisionExtension ? "-" : "",
105-
mapcodeElements.precisionExtension);
100+
snprintf(romanized, sizeof(romanized), "%s%s%s%s%s",
101+
mapcodeElements.territoryISO,
102+
*mapcodeElements.territoryISO ? " " : "",
103+
mapcodeElements.properMapcode,
104+
*mapcodeElements.precisionExtension ? "-" : "",
105+
mapcodeElements.precisionExtension);
106106
if ((int)strlen(romanized) < maxLength) {
107107
strcpy(asciiBuffer, romanized);
108108
}

mapcodelib/mapcoder.c

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -534,17 +534,11 @@ static Point convertFractionsToDegrees(const Point* p) {
534534
}
535535

536536

537-
static const unsigned char DOUBLE_NAN[8] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x7F}; // NAN (Not a Number)
538-
static const unsigned char DOUBLE_INF[8] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xF0, 0x7F}; // +Infinity
539-
static const unsigned char DOUBLE_MIN_INF[8] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xF0, 0xFF}; // -Infinity
540-
541537
static enum MapcodeError
542538
convertCoordsToMicrosAndFractions(Point32* coord32, int* fracLat, int* fracLon, double latDeg, double lonDeg) {
543539
double frac;
544540
ASSERT(coord32);
545-
if (memcmp(&lonDeg, DOUBLE_NAN, 8) == 0 || memcmp(&lonDeg, DOUBLE_INF, 8) == 0 ||
546-
memcmp(&lonDeg, DOUBLE_MIN_INF, 8) == 0 ||
547-
memcmp(&latDeg, DOUBLE_NAN, 8) == 0) {
541+
if (isnan(lonDeg) || isinf(lonDeg) || isnan(latDeg) || isinf(latDeg)) {
548542
return ERR_BAD_COORDINATE;
549543
}
550544
if (latDeg < -90) {
@@ -1081,7 +1075,6 @@ static void encodeExtension(char* result, const int extrax4, const int extray, c
10811075
valy -= factory * gy; // for next iteration
10821076
}
10831077
*s = 0; // terminate the result
1084-
ASSERT((int) strlen(s) == extraDigits);
10851078
}
10861079
}
10871080

@@ -1487,7 +1480,7 @@ static void encoderEngine(const enum Territory ccode, const EncodeRec* enc, cons
14871480
///////////////////////////////////////////////////////////
14881481
{
14891482
int i;
1490-
char result[128];
1483+
char result[MAX_MAPCODE_RESULT_ASCII_LEN];
14911484
int result_counter = 0;
14921485

14931486
*result = 0;
@@ -1736,7 +1729,7 @@ static int decodeBase31(const char* code) {
17361729
static void decodeTriple(const char* result, int* difx, int* dify) {
17371730
// decode the first character
17381731
const int c1 = decodeChar(*result++);
1739-
ASSERT(result);
1732+
ASSERT(result - 1);
17401733
ASSERT(difx);
17411734
ASSERT(dify);
17421735
if (c1 < 24) {
@@ -2125,7 +2118,10 @@ static unsigned char getRomanVersionOf(UWORD w) {
21252118
static void convertFromAbjad(char* s) {
21262119
int len, dot, form, c;
21272120
char* postfix = strchr(s, '-');
2128-
dot = (int)(strchr(s, '.') - s);
2121+
{
2122+
const char* dotptr = strchr(s, '.');
2123+
dot = dotptr ? (int)(dotptr - s) : -1;
2124+
}
21292125
if (dot < 2 || dot > 5) {
21302126
return;
21312127
}
@@ -3020,7 +3016,10 @@ static char* convertToAbjad(char* targetAsciiString, const char* sourceAsciiStri
30203016
unpackIfAllDigits(targetAsciiString);
30213017

30223018
len = (int)strlen(targetAsciiString);
3023-
dot = (int)(strchr(targetAsciiString, '.') - targetAsciiString);
3019+
{
3020+
const char* dotptr = strchr(targetAsciiString, '.');
3021+
dot = dotptr ? (int)(dotptr - targetAsciiString) : -1;
3022+
}
30243023

30253024
form = dot * 10 + (len - dot - 1);
30263025

@@ -3297,6 +3296,7 @@ UWORD* convertToAlphabet(UWORD* utf16String, int maxLength, const char* asciiStr
32973296
* Convert a zero-terminated UTF16 to a UTF8 string
32983297
*/
32993298
char* convertUtf16ToUtf8(char* utf8, const UWORD* utf16) {
3299+
char* start = utf8;
33003300
ASSERT(utf16);
33013301
ASSERT(utf8);
33023302
while (*utf16) {
@@ -3315,7 +3315,7 @@ char* convertUtf16ToUtf8(char* utf8, const UWORD* utf16) {
33153315
}
33163316
}
33173317
*utf8 = 0;
3318-
return utf8;
3318+
return start;
33193319
}
33203320

33213321
// Caller must make sure utf8String can hold at least MAX_MAPCODE_RESULT_LEN characters (including 0-terminator).
@@ -3585,9 +3585,6 @@ encodeLatLonToSingleMapcode(char* mapcode, double latDeg, double lonDeg, enum Te
35853585
if (extraDigits > MAX_PRECISION_DIGITS) {
35863586
extraDigits = MAX_PRECISION_DIGITS;
35873587
}
3588-
if (territory <= TERRITORY_UNKNOWN) {
3589-
return 0;
3590-
}
35913588
ret = encodeLatLonToMapcodes_internal(&rlocal, latDeg, lonDeg, territory, 1, DEBUG_STOP_AT, extraDigits);
35923589
*mapcode = 0;
35933590
if (ret <= 0) {
@@ -3608,7 +3605,7 @@ encodeLatLonToSelectedMapcode(char* mapcode, double latDeg, double lonDeg, enum
36083605
int nrOfResults = 0;
36093606
nrOfResults = encodeLatLonToMapcodes(&mapcodes, latDeg, lonDeg, territory, extraDigits);
36103607
ASSERT(nrOfResults == mapcodes.count);
3611-
if ((nrOfResults <= 0) || (indexOfSelected < 0) || (indexOfSelected > nrOfResults)) {
3608+
if ((nrOfResults <= 0) || (indexOfSelected < 0) || (indexOfSelected >= nrOfResults)) {
36123609
return 0;
36133610
}
36143611
strcpy(mapcode, mapcodes.mapcode[indexOfSelected]);

mapcodelib/mapcoder.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ extern "C" {
2020

2121
#endif
2222

23+
#include <stdint.h>
2324
#include "mapcode_territories.h"
2425
#include "mapcode_alphabets.h"
2526

@@ -58,7 +59,7 @@ extern "C" {
5859
#endif
5960

6061
#define MAPCODE_C_VERSION "2.5.6"
61-
#define UWORD unsigned short int // 2-byte unsigned integer.
62+
#define UWORD uint16_t // 2-byte unsigned integer.
6263

6364
#define MAX_NR_OF_MAPCODE_RESULTS 22 // Max. number of results ever returned by encoder (e.g. for 26.904899, 95.138515).
6465

test/unittest.c

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2222,6 +2222,78 @@ static int testAlphabetPerTerritory(void) {
22222222
}
22232223

22242224

2225+
static int testBugFixes(void) {
2226+
int nrTests = 0;
2227+
2228+
// Issue 1: lat=+Inf or lat=-Inf must return ERR_BAD_COORDINATE (0 results), same as lon=Inf
2229+
{
2230+
Mapcodes mapcodes;
2231+
double pos_inf = 1.0 / 0.0;
2232+
double neg_inf = -1.0 / 0.0;
2233+
int n;
2234+
2235+
n = encodeLatLonToMapcodes(&mapcodes, pos_inf, 0.0, TERRITORY_NONE, 0);
2236+
++nrTests;
2237+
if (n != 0) {
2238+
foundError();
2239+
printf("*** ERROR *** encodeLatLonToMapcodes(lat=+Inf) should return 0, got %d\n", n);
2240+
}
2241+
2242+
n = encodeLatLonToMapcodes(&mapcodes, neg_inf, 0.0, TERRITORY_NONE, 0);
2243+
++nrTests;
2244+
if (n != 0) {
2245+
foundError();
2246+
printf("*** ERROR *** encodeLatLonToMapcodes(lat=-Inf) should return 0, got %d\n", n);
2247+
}
2248+
}
2249+
2250+
// Issue 2: indexOfSelected == nrOfResults is out-of-bounds and must return 0
2251+
{
2252+
char mapcode[MAX_MAPCODE_RESULT_ASCII_LEN];
2253+
// lat=52.158993, lon=4.492346 yields 4 results (verified in testSelectedEncodes)
2254+
int n = encodeLatLonToSelectedMapcode(mapcode, 52.158993, 4.492346, TERRITORY_NONE, 0, 4);
2255+
++nrTests;
2256+
if (n != 0) {
2257+
foundError();
2258+
printf("*** ERROR *** encodeLatLonToSelectedMapcode with indexOfSelected==nrOfResults should return 0, got %d\n", n);
2259+
}
2260+
}
2261+
2262+
// Issue 7: encodeLatLonToSingleMapcode must accept TERRITORY_NONE and TERRITORY_UNKNOWN
2263+
{
2264+
char mapcode[MAX_MAPCODE_RESULT_ASCII_LEN];
2265+
int n;
2266+
2267+
n = encodeLatLonToSingleMapcode(mapcode, 52.3, 4.9, TERRITORY_UNKNOWN, 0);
2268+
++nrTests;
2269+
if (n <= 0) {
2270+
foundError();
2271+
printf("*** ERROR *** encodeLatLonToSingleMapcode(TERRITORY_UNKNOWN) should succeed, got %d\n", n);
2272+
}
2273+
2274+
n = encodeLatLonToSingleMapcode(mapcode, 52.3, 4.9, TERRITORY_NONE, 0);
2275+
++nrTests;
2276+
if (n <= 0) {
2277+
foundError();
2278+
printf("*** ERROR *** encodeLatLonToSingleMapcode(TERRITORY_NONE) should succeed, got %d\n", n);
2279+
}
2280+
}
2281+
2282+
// Issue 9: convertMapcodeToAlphabetUtf8 must return the start of the output buffer
2283+
{
2284+
char utf8[MAX_MAPCODE_RESULT_UTF8_LEN + 1];
2285+
char *result = convertMapcodeToAlphabetUtf8(utf8, "NLD 49.4V", ALPHABET_ROMAN);
2286+
++nrTests;
2287+
if (result != utf8) {
2288+
foundError();
2289+
printf("*** ERROR *** convertMapcodeToAlphabetUtf8 must return start of buffer, got wrong pointer\n");
2290+
}
2291+
}
2292+
2293+
return nrTests;
2294+
}
2295+
2296+
22252297
int main(const int argc, const char **argv) {
22262298
int nrTests = 0;
22272299

@@ -2281,6 +2353,9 @@ int main(const int argc, const char **argv) {
22812353
printf("-----------------------------------------------------------\nRe-encode tests\n");
22822354
nrTests += testReEncode();
22832355

2356+
printf("-----------------------------------------------------------\nBug-fix regression tests\n");
2357+
nrTests += testBugFixes();
2358+
22842359
printf("-----------------------------------------------------------\n");
22852360
printf("Done.\nExecuted %d tests, found %d errors\n", nrTests, nrErrors);
22862361
if (nrErrors > 0) {

0 commit comments

Comments
 (0)