MITAB .tab: AlterFieldDefn(): fix data corruption when altering (for …

autotest/ogr/ogr_mitab.py

@@ -2812,3 +2812,109 @@ def test_ogr_mitab_label_without_text(tmp_vsimem):
 def test_ogr_mitab_write_LCC_2SP_non_metre_unit(tmp_vsimem, ext):
 
     _test_srs(tmp_vsimem, "EPSG:2277", ext=ext)  # "NAD83 / Texas Central (ftUS)"
+
+
+###############################################################################
+
+
+def test_ogr_mitab_alter_field_defn_integer_width_4(tmp_vsimem):
+
+    filename = str(tmp_vsimem / "foo.tab")
+
+    ds = ogr.GetDriverByName("MapInfo File").CreateDataSource(filename)
+    lyr = ds.CreateLayer("foo")
+    fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger)
+    fld_defn.SetWidth(5)
+    lyr.CreateField(fld_defn)
+
+    # Changing field defn while no feature has been written is OK
+    idx = lyr.GetLayerDefn().GetFieldIndex("int_field")
+
+    new_fld_defn = ogr.FieldDefn("real_field", ogr.OFTReal)
+    new_fld_defn.SetWidth(15)
+    new_fld_defn.SetPrecision(6)
+    assert lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
+    fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
+    assert fld_defn.GetType() == ogr.OFTReal
+    assert fld_defn.GetWidth() == 15
+    assert fld_defn.GetPrecision() == 6
+
+    new_fld_defn = ogr.FieldDefn("real_field", ogr.OFTReal)
+    new_fld_defn.SetWidth(30)
+    new_fld_defn.SetPrecision(6)
+    assert lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
+    fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
+    assert fld_defn.GetType() == ogr.OFTReal
+    assert fld_defn.GetWidth() == 20
+    assert fld_defn.GetPrecision() == 6
+
+    new_fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger)
+    # Use 4 as this is also sizeof(int32) and there was a confusion before
+    # the fix linked with that test between decimal width and binary width.
+    new_fld_defn.SetWidth(4)
+    assert lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
+    fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
+    assert fld_defn.GetType() == ogr.OFTInteger
+    assert fld_defn.GetWidth() == 4
+
+    f = ogr.Feature(lyr.GetLayerDefn())
+    f["int_field"] = 1234
+    lyr.CreateFeature(f)
+    f = None
+
+    new_fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger64)
+    with gdal.quiet_errors():
+        assert (
+            lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_ALL_FLAG) != ogr.OGRERR_NONE
+        )
+    fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
+    assert fld_defn.GetType() == ogr.OFTInteger
+    assert fld_defn.GetWidth() == 4
+
+    ds = None
+
+    ds = ogr.Open(filename, update=1)
+    lyr = ds.GetLayer(0)
+    idx = lyr.GetLayerDefn().GetFieldIndex("int_field")
+    fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
+    assert fld_defn.GetWidth() == 4
+    # We don't change anything actually
+    assert lyr.AlterFieldDefn(idx, fld_defn, ogr.ALTER_ALL_FLAG) == ogr.OGRERR_NONE
+    fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
+    assert fld_defn.GetType() == ogr.OFTInteger
+    assert fld_defn.GetWidth() == 4
+    f = lyr.GetNextFeature()
+    assert f["int_field"] == 1234
+
+
+###############################################################################
+
+
+def test_ogr_mitab_alter_field_defn_to_string(tmp_vsimem):
+
+    filename = str(tmp_vsimem / "foo.tab")
+
+    ds = ogr.GetDriverByName("MapInfo File").CreateDataSource(filename)
+    lyr = ds.CreateLayer("foo")
+    fld_defn = ogr.FieldDefn("int_field", ogr.OFTInteger)
+    fld_defn.SetWidth(5)
+    lyr.CreateField(fld_defn)
+    f = ogr.Feature(lyr.GetLayerDefn())
+    f["int_field"] = 1234
+    lyr.CreateFeature(f)
+    f = None
+    ds = None
+
+    ds = ogr.Open(filename, update=1)
+    lyr = ds.GetLayer(0)
+    idx = lyr.GetLayerDefn().GetFieldIndex("int_field")
+    new_fld_defn = ogr.FieldDefn("str_field", ogr.OFTString)
+    assert (
+        lyr.AlterFieldDefn(idx, new_fld_defn, ogr.ALTER_NAME_FLAG | ogr.ALTER_TYPE_FLAG)
+        == ogr.OGRERR_NONE
+    )
+    fld_defn = lyr.GetLayerDefn().GetFieldDefn(idx)
+    assert fld_defn.GetType() == ogr.OFTString
+    assert fld_defn.GetWidth() == 254
+    f = lyr.GetNextFeature()
+    assert f["str_field"] == "1234"

ogr/ogrsf_frmts/mitab/mitab_datfile.cpp

@@ -748,19 +748,20 @@ static int TABDATFileSetFieldDefinition(TABDATFieldDef *psFieldDef,
     else if (nWidth == 0)
         nWidth = 254;  // char fields.
 
-    strncpy(psFieldDef->szName, pszName, sizeof(psFieldDef->szName) - 1);
-    psFieldDef->szName[sizeof(psFieldDef->szName) - 1] = '\0';
+    snprintf(psFieldDef->szName, sizeof(psFieldDef->szName), "%s", pszName);
     psFieldDef->eTABType = eType;
-    psFieldDef->byLength = static_cast<GByte>(nWidth);
-    psFieldDef->byDecimals = static_cast<GByte>(nPrecision);
+    psFieldDef->byDecimals = 0;
 
     switch (eType)
     {
         case TABFChar:
             psFieldDef->cType = 'C';
+            psFieldDef->byLength = static_cast<GByte>(nWidth);
             break;
         case TABFDecimal:
             psFieldDef->cType = 'N';
+            psFieldDef->byLength = static_cast<GByte>(nWidth);
+            psFieldDef->byDecimals = static_cast<GByte>(nPrecision);
             break;
         case TABFInteger:
             psFieldDef->cType = 'C';
@@ -1236,8 +1237,8 @@ int TABDATFile::ReorderFields(int *panMap)
 /*                           AlterFieldDefn()                           */
 /************************************************************************/
 
-int TABDATFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
-                               int nFlags)
+int TABDATFile::AlterFieldDefn(int iField, const OGRFieldDefn *poSrcFieldDefn,
+                               OGRFieldDefn *poNewFieldDefn, int nFlags)
 {
     if (m_fp == nullptr)
     {
@@ -1261,43 +1262,45 @@ int TABDATFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
     }
 
     TABFieldType eTABType = m_pasFieldDef[iField].eTABType;
-    int nWidth = m_pasFieldDef[iField].byLength;
-    int nPrecision = m_pasFieldDef[iField].byDecimals;
-    int nWidthDummy = 0;
-    int nPrecisionDummy = 0;
+    int nWidth = poSrcFieldDefn->GetWidth();
+    int nPrecision = poSrcFieldDefn->GetPrecision();
     if (nFlags & ALTER_TYPE_FLAG)
     {
-        if (IMapInfoFile::GetTABType(poNewFieldDefn, &eTABType, &nWidthDummy,
-                                     &nPrecisionDummy) < 0)
+        if (IMapInfoFile::GetTABType(poNewFieldDefn, &eTABType, nullptr,
+                                     nullptr) < 0)
             return -1;
     }
     if (nFlags & ALTER_WIDTH_PRECISION_FLAG)
     {
-        TABFieldType eTABTypeDummy;
-        if (IMapInfoFile::GetTABType(poNewFieldDefn, &eTABTypeDummy, &nWidth,
+        // Instead of taking directly poNewFieldDefn->GetWidth()/GetPrecision(),
+        // use GetTABType() to take into account .dat limitations on
+        // width & precision to clamp what user might have specify
+        if (IMapInfoFile::GetTABType(poNewFieldDefn, nullptr, &nWidth,
                                      &nPrecision) < 0)
             return -1;
     }
 
     if ((nFlags & ALTER_TYPE_FLAG) &&
         eTABType != m_pasFieldDef[iField].eTABType)
     {
-        if (eTABType != TABFChar)
+        if (eTABType != TABFChar && m_numRecords > 0)
         {
             CPLError(CE_Failure, CPLE_NotSupported,
                      "Can only convert to OFTString");
             return -1;
         }
-        if ((nFlags & ALTER_WIDTH_PRECISION_FLAG) == 0)
+        if (eTABType == TABFChar && (nFlags & ALTER_WIDTH_PRECISION_FLAG) == 0)
             nWidth = 254;
     }
 
     if (nFlags & ALTER_WIDTH_PRECISION_FLAG)
     {
-        if (eTABType != TABFChar && nWidth != m_pasFieldDef[iField].byLength)
+        if (eTABType != TABFChar && nWidth != poSrcFieldDefn->GetWidth() &&
+            m_numRecords > 0)
         {
-            CPLError(CE_Failure, CPLE_NotSupported,
-                     "Resizing only supported on String fields");
+            CPLError(
+                CE_Failure, CPLE_NotSupported,
+                "Resizing only supported on String fields on non-empty layer");
             return -1;
         }
     }
@@ -1330,12 +1333,31 @@ int TABDATFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
         }
         if (nFlags & ALTER_WIDTH_PRECISION_FLAG)
         {
-            m_pasFieldDef[iField].byLength = static_cast<GByte>(nWidth);
-            m_pasFieldDef[iField].byDecimals = static_cast<GByte>(nPrecision);
+            if (eTABType == TABFChar || eTABType == TABFDecimal)
+                m_pasFieldDef[iField].byLength = static_cast<GByte>(nWidth);
+            if (eTABType == TABFDecimal)
+                m_pasFieldDef[iField].byDecimals =
+                    static_cast<GByte>(nPrecision);
         }
         return 0;
     }
 
+    const bool bWidthPrecisionPreserved =
+        (nWidth == poSrcFieldDefn->GetWidth() &&
+         nPrecision == poSrcFieldDefn->GetPrecision());
+    if (eTABType == m_pasFieldDef[iField].eTABType && bWidthPrecisionPreserved)
+    {
+        return 0;
+    }
+
+    if (eTABType != TABFChar)
+    {
+        // should hopefully not happen given all above checks
+        CPLError(CE_Failure, CPLE_NotSupported,
+                 "Unsupported AlterFieldDefn() operation");
+        return -1;
+    }
+
     // Otherwise we need to do a temporary file.
     TABDATFile oTempFile(GetEncoding());
     CPLString osOriginalFile(m_pszFname);

ogr/ogrsf_frmts/mitab/mitab_imapinfofile.cpp

@@ -441,7 +441,8 @@ int IMapInfoFile::GetTABType(const OGRFieldDefn *poField,
 {
     TABFieldType eTABType;
     int nWidth = poField->GetWidth();
-    int nPrecision = poField->GetPrecision();
+    int nPrecision =
+        poField->GetType() == OFTReal ? poField->GetPrecision() : 0;
 
     if (poField->GetType() == OFTInteger)
     {
@@ -519,9 +520,12 @@ int IMapInfoFile::GetTABType(const OGRFieldDefn *poField,
         return -1;
     }
 
-    *peTABType = eTABType;
-    *pnWidth = nWidth;
-    *pnPrecision = nPrecision;
+    if (peTABType)
+        *peTABType = eTABType;
+    if (pnWidth)
+        *pnWidth = nWidth;
+    if (pnPrecision)
+        *pnPrecision = nPrecision;
 
     return 0;
 }

ogr/ogrsf_frmts/mitab/mitab_priv.h

@@ -231,7 +231,8 @@ typedef struct TABDATFieldDef_t
 {
     char szName[11];
     char cType;
-    GByte byLength;
+    GByte
+        byLength; /* caution: for a native .dat file, this is a binary width for most types */
     GByte byDecimals;
 
     TABFieldType eTABType;
@@ -1876,7 +1877,8 @@ class TABDATFile
 
     int DeleteField(int iField);
     int ReorderFields(int *panMap);
-    int AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn, int nFlags);
+    int AlterFieldDefn(int iField, const OGRFieldDefn *poSrcFieldDefn,
+                       OGRFieldDefn *poNewFieldDefn, int nFlags);
 
     int SyncToDisk();
 

ogr/ogrsf_frmts/mitab/mitab_tabfile.cpp

@@ -2839,18 +2839,17 @@ OGRErr TABFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
         return OGRERR_FAILURE;
     }
 
-    if (m_poDATFile->AlterFieldDefn(iField, poNewFieldDefn, nFlagsIn) == 0)
+    OGRFieldDefn *poFieldDefn = m_poDefn->GetFieldDefn(iField);
+    if (m_poDATFile->AlterFieldDefn(iField, poFieldDefn, poNewFieldDefn,
+                                    nFlagsIn) == 0)
     {
         m_bNeedTABRewrite = TRUE;
 
-        OGRFieldDefn *poFieldDefn = m_poDefn->GetFieldDefn(iField);
         auto oTemporaryUnsealer(poFieldDefn->GetTemporaryUnsealer());
         if ((nFlagsIn & ALTER_TYPE_FLAG) &&
             poNewFieldDefn->GetType() != poFieldDefn->GetType())
         {
             poFieldDefn->SetType(poNewFieldDefn->GetType());
-            if ((nFlagsIn & ALTER_WIDTH_PRECISION_FLAG) == 0)
-                poFieldDefn->SetWidth(254);
         }
         if (nFlagsIn & ALTER_NAME_FLAG)
         {
@@ -2859,11 +2858,23 @@ OGRErr TABFile::AlterFieldDefn(int iField, OGRFieldDefn *poNewFieldDefn,
             m_oSetFields.insert(
                 CPLString(poNewFieldDefn->GetNameRef()).toupper());
         }
-        if ((nFlagsIn & ALTER_WIDTH_PRECISION_FLAG) &&
-            poFieldDefn->GetType() == OFTString)
+        if (poFieldDefn->GetType() == OFTString)
         {
             poFieldDefn->SetWidth(m_poDATFile->GetFieldWidth(iField));
         }
+        else if (nFlagsIn & ALTER_WIDTH_PRECISION_FLAG)
+        {
+            poFieldDefn->SetWidth(poNewFieldDefn->GetWidth());
+            poFieldDefn->SetPrecision(poNewFieldDefn->GetPrecision());
+        }
+
+        // Take into account .dat limitations on width & precision to clamp
+        // what user might have specify
+        int nWidth = 0;
+        int nPrecision = 0;
+        GetTABType(poFieldDefn, nullptr, &nWidth, &nPrecision);
+        poFieldDefn->SetWidth(nWidth);
+        poFieldDefn->SetPrecision(nPrecision);
 
         if (m_eAccessMode == TABReadWrite)
             WriteTABFile();