msgfmt segfaults building git 2.19.1

[rofl0r]

to reproduce:

msgfmt --check --statistics -o ru.mo po/ru.po

resulting in:

Program received signal SIGSEGV, Segmentation fault.
0x000000000040273c in process (in=0x605340, out=0x7ffff7ffb020)
    at src/msgfmt.c:471
471                     d.strlist[i].trans->off += d.off + d.len[pe_msgid] + d.len[pe_plural] + d.len[pe_ctxt];
(gdb) p i
$1 = 1
(gdb) p d.strlist[i]
$2 = {str = {len = 0, off = 53980}, trans = 0x0}
(gdb) p d.strlist[i-1]
$3 = {str = {len = 0, off = 53980}, trans = 0x619480}

the trans pointer of the second item is NULL. interestingly the 2nd item seems identical to the first apart from the trans pointer. i guess it's from the empty msgid "" string right at the start of the file.

@xhebox do you have time to look into this ?

[xhebox]

@rofl0r I can take a try, but i will let you know if i can not complete it under this broken system. I just escape from the busy and hard time as a freshman. And i'm currently busy on fixing my broken toolchain and broken distribution.

[xhebox]

@rofl0r here's why:

msgid "ordered %u objects, expected %<PRIu32>"
msgstr ""

Here, a sysdep msgid, but an empty msgstr, resulting in a mismatched num of msgid/msgstr(msgid - msgstr == 1). And my dirty hack to drop empty msgstr translation makes the num equal improperly, so abort() is avoided, and segfault came naturally.

We really need a good logic in process_line_callback, so that we can drop an invalid translation anytime we want. One has came up to my mind: we could use a struct to record the whole ctxt/id/plural/str process when collect_size/copy_string, rather than playing with just a priv_type that could only tell me what previous statement is...

This actually reminds me that the poparser is also a bit tricky... These issues occurred over and over again on gettext-tiny. Not a surprise, we can not use hacks to meet the needs anymore. So i propose a new poparser, which:

1. parses po files into blocks. Each block corresponds to a msgid, a possible msgctxt/msgid_plural, and possibly multiple msgstr, and some recognizable comments(start with #:, #,) generated by xgettext. Blank lines and normal comments are ignored.

2. is not using action_tbl in feed_line() and not calling the cb_func everytime a newline is fed. action_tblis just a 2d array, it can just care about two states. But we have to care about at least four states(ctxt/id/plural/str). We need to care about the whole block, or we need a bigger state machine in another word.

We should just parse the new lines, check if the block is terminated, and leave the all work to the callback function once a new block is parsed.

[rofl0r]

msgid "ordered %u objects, expected %"
msgstr ""

what does gnu gettext do with this one ?

[xhebox]

Drop it. Empty msgstr is ignored by default. -C will trigger an error, since X/Open format strictly not allow the existence of emty msgstr. But to be honest, empty msgstr makes translation more convenient for translators.

[rofl0r]

ok. do you have time to work on your suggested poparser modifications ? i suspect it should not be too much work...

[xhebox]

I have. No more than 1k loc, i guess. One week should be OK. Since i have other things in the meantime.

[rofl0r]

1000 lines ? i hope not :)
joking aside, that's really good news, thanks!

[xhebox]

@rofl0r i've opened a new branch. actually the newpoparser managed to reduce some codes... msgfmt greatly reduced by 243 while poparser.c increased by 152.

let's go back to the main topic. i only completed the work of poparser and msgfmt this weekend. msgmerge, though, is quite simple to rewrite, i have to leave it for the next weekend(or earlier). you could also try to rewrite, since msgmerge is just doing copy. you could test the new msgfmt freely. bug is welcome.

besides, the new parser is completely capable of parsing po header and flags in comments. i only parse charset, nplurals, fuzzy as the old one, but we could now extend those with ease.

[rofl0r]

great job. i left a number of comments on your commits. i think you oversimplified the sysdep replacement code, and the version you provide is unfit for anything but the most simple cases. imo it should be possible to fit in the previous code, which i tested rigorously back when i wrote it. anyway, i'm waiting for your comments...

[rofl0r]

for the record, this was the commit adding the sysdep expansion: 438a47c

[xhebox]

1. cmake version string. i forgot to pull the newest commits before coding. and i picked the wrong code when rebasing.

2. indent problems. i've switched to spacevim, but somehow my vim defaults to softtabs(spaces). also, it causes some useless indents while i use '='.

3. sysdep. imo, it's good enough, though i just found a bug when i typed this comment. i dont know why your codes will be so complicated, maybe you have a 'nice' misunderstanding on the problem. let me give a full description on my solution.

first, our problem: replace %<xxx> with %x in short. it's straight forward to just copy when there's only one %<xxx>. what we need to consider is the case where there're multiple %<xxx>.

let case(x) be a function that accepts %<xxx> as the input and outputs a set of specifiers like {u, lu}. we will need to do a mulplication to collect every case: case(%<xxx1>) x case(%<xxx2>) x ... x case(%<xxxn>) = {cartesian product}.

but attention, we dont need to do mulplication for duplicated %<xxx>, e.g. no case(%<PRIu32>) x case(%<PRIu32>). because, how could there be a platform where PRIu32 could be u and lu at the same time?

second, my solution: i want my sysdep() be a function like unescape() and does not contain malloc().

according to the above analysis, i dont need to replace identical sysdep strings into different specifiers. so for every case, sysdep() just need to pick up the right specifier for the correspoding sysdep string. and just search for the sysdep string, copy the specifier and skip the sysdep string and so on.

and the trick is that i use the num of case to pick up the right specifier. there're only up to four cases: case(%<PRIu32>) x case(%<PRIu64>) x case(%<PRIuMAX>) = { {u, lu, ju}, {u,llu,ju}, {lu,lu,ju}, {lu,llu,ju} }. and %<PRIuMAX> wont change all the time. so 4 cases corresponds to a two-bit flag control %<PRIu32> and %<PRIu64>. but this fails, since i wont know what 2-case stand for 32 or 64...

third. how to fix bug: we could replace msg->sysdep with an array sysdep[3](which adds to sysdep). in this way, we could iterate like:

callback(in, out, n) {

n[3] = {0};
for (k=true;k;) {
  k = false;
  sysdep(in, out,  n);

  for (i=0; i<3; i++) {
     if (n[i] < (sysdep[i]-1)) {
        n[i]++;
        k= true;
        break;
     }
  }
}

}

4. i currently have no time to code. i could get enough time to solve problems after at least two days. i'd like to leave the work to you, or just wait for me. never mind, i found i need to correct tiny to update my broken distribution, so i have pushed new commits.

[rofl0r]

but attention, we dont need to do mulplication for duplicated %, e.g. no case(%) x case(%). because, how could there be a platform where PRIu32 could be u and lu at the same time?

our produced .mo files need to be portable across systems.
that means it's possible to create a .mo file on x86_64, but use it on i386.
therefore an input string containing e.g. PRIu64, will depending on the libc of the user, generate either "lu" or "llu", so our generated .mo file need to create one msgid string with "lu" and another one with "llu" to have all bases covered. if there's more than one sysdep string, this leads to the combinatorial explosion which my existing code covers correctly.

[xhebox]

@rofl0r the new branch, whatever, passed my test. i'd like to hear a reply from you.